From d32665af9ddab62c201af820bfdf8fc2d5cd3e6f Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sun, 10 Jun 2018 17:02:06 +0200 Subject: [PATCH] remove FHL2 experiment FHL2 was an experiment in Chrome, run in Q036, which introduced HOL blocking. This experiment is over, so we can remove the code to send a PUBLIC_RESET when a peer initiates this experiment. --- internal/handshake/crypto_setup_server.go | 8 -------- internal/handshake/crypto_setup_server_test.go | 11 ----------- internal/handshake/tags.go | 4 ---- session.go | 1 - session_test.go | 11 +---------- 5 files changed, 1 insertion(+), 34 deletions(-) diff --git a/internal/handshake/crypto_setup_server.go b/internal/handshake/crypto_setup_server.go index 952237e8..552e8297 100644 --- a/internal/handshake/crypto_setup_server.go +++ b/internal/handshake/crypto_setup_server.go @@ -60,11 +60,6 @@ type cryptoSetupServer struct { var _ CryptoSetup = &cryptoSetupServer{} -// ErrHOLExperiment is returned when the client sends the FHL2 tag in the CHLO. -// This is an experiment implemented by Chrome in QUIC 36, which we don't support. -// TODO: remove this when dropping support for QUIC 36 -var ErrHOLExperiment = qerr.Error(qerr.InvalidCryptoMessageParameter, "HOL experiment. Unsupported") - // ErrNSTPExperiment is returned when the client sends the NSTP tag in the CHLO. // This is an experiment implemented by Chrome in QUIC 38, which we don't support at this point. var ErrNSTPExperiment = qerr.Error(qerr.InvalidCryptoMessageParameter, "NSTP experiment. Unsupported") @@ -132,9 +127,6 @@ func (h *cryptoSetupServer) HandleCryptoStream() error { } func (h *cryptoSetupServer) handleMessage(chloData []byte, cryptoData map[Tag][]byte) (bool, error) { - if _, isHOLExperiment := cryptoData[TagFHL2]; isHOLExperiment { - return false, ErrHOLExperiment - } if _, isNSTPExperiment := cryptoData[TagNSTP]; isNSTPExperiment { return false, ErrNSTPExperiment } diff --git a/internal/handshake/crypto_setup_server_test.go b/internal/handshake/crypto_setup_server_test.go index 15624f84..e5833edf 100644 --- a/internal/handshake/crypto_setup_server_test.go +++ b/internal/handshake/crypto_setup_server_test.go @@ -210,17 +210,6 @@ var _ = Describe("Server Crypto Setup", func() { } }) - It("doesn't support Chrome's head-of-line blocking experiment", func() { - HandshakeMessage{ - Tag: TagCHLO, - Data: map[Tag][]byte{ - TagFHL2: []byte("foobar"), - }, - }.Write(&stream.dataToRead) - err := cs.HandleCryptoStream() - Expect(err).To(MatchError(ErrHOLExperiment)) - }) - It("doesn't support Chrome's no STOP_WAITING experiment", func() { HandshakeMessage{ Tag: TagCHLO, diff --git a/internal/handshake/tags.go b/internal/handshake/tags.go index 19ec78d3..cf2a7562 100644 --- a/internal/handshake/tags.go +++ b/internal/handshake/tags.go @@ -50,10 +50,6 @@ const ( // TagSFCW is the initial stream flow control receive window. TagSFCW Tag = 'S' + 'F'<<8 + 'C'<<16 + 'W'<<24 - // TagFHL2 forces head of line blocking. - // Chrome experiment (see https://codereview.chromium.org/2115033002) - // unsupported by quic-go - TagFHL2 Tag = 'F' + 'H'<<8 + 'L'<<16 + '2'<<24 // TagNSTP is the no STOP_WAITING experiment // currently unsupported by quic-go TagNSTP Tag = 'N' + 'S'<<8 + 'T'<<16 + 'P'<<24 diff --git a/session.go b/session.go index 957dfae1..0c2f3beb 100644 --- a/session.go +++ b/session.go @@ -868,7 +868,6 @@ func (s *session) handleCloseError(closeErr closeError) error { } if quicErr.ErrorCode == qerr.DecryptionFailure || - quicErr == handshake.ErrHOLExperiment || quicErr == handshake.ErrNSTPExperiment { return s.sendPublicReset(s.lastRcvdPacketNumber) } diff --git a/session_test.go b/session_test.go index f422001f..80240f0e 100644 --- a/session_test.go +++ b/session_test.go @@ -550,19 +550,10 @@ var _ = Describe("Session", func() { Expect(mconn.written).To(BeEmpty()) // no CONNECTION_CLOSE or PUBLIC_RESET sent }) - It("sends a Public Reset if the client is initiating the head-of-line blocking experiment", func() { - streamManager.EXPECT().CloseWithError(gomock.Any()) - sessionRunner.EXPECT().removeConnectionID(gomock.Any()) - sess.Close(handshake.ErrHOLExperiment) - Expect(mconn.written).To(HaveLen(1)) - Expect((<-mconn.written)[0] & 0x02).ToNot(BeZero()) // Public Reset - Expect(sess.Context().Done()).To(BeClosed()) - }) - It("sends a Public Reset if the client is initiating the no STOP_WAITING experiment", func() { streamManager.EXPECT().CloseWithError(gomock.Any()) sessionRunner.EXPECT().removeConnectionID(gomock.Any()) - sess.Close(handshake.ErrHOLExperiment) + sess.Close(handshake.ErrNSTPExperiment) Expect(mconn.written).To(HaveLen(1)) Expect((<-mconn.written)[0] & 0x02).ToNot(BeZero()) // Public Reset Expect(sess.Context().Done()).To(BeClosed())