From 63843cc17e2d4d4dcb105cf8f846c657125fdae5 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 28 Nov 2018 14:06:54 +0700 Subject: [PATCH 1/3] close the underlying connection when the context is canceled --- p2p/security/tls/transport.go | 32 +++++++++++++++++++++--- p2p/security/tls/transport_test.go | 40 ++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/p2p/security/tls/transport.go b/p2p/security/tls/transport.go index 16941bc0..40181f9a 100644 --- a/p2p/security/tls/transport.go +++ b/p2p/security/tls/transport.go @@ -43,8 +43,20 @@ var _ cs.Transport = &Transport{} // SecureInbound runs the TLS handshake as a server. func (t *Transport) SecureInbound(ctx context.Context, insecure net.Conn) (cs.Conn, error) { serv := tls.Server(insecure, t.identity.Config) - // TODO: use the ctx - // see https://github.com/golang/go/issues/18482 + + // There's no way to pass a context to tls.Conn.Handshake(). + // See https://github.com/golang/go/issues/18482. + // Close the connection instead. + done := make(chan struct{}) + defer close(done) + go func() { + select { + case <-done: + case <-ctx.Done(): + insecure.Close() + } + }() + if err := serv.Handshake(); err != nil { return nil, err } @@ -54,8 +66,20 @@ func (t *Transport) SecureInbound(ctx context.Context, insecure net.Conn) (cs.Co // SecureOutbound runs the TLS handshake as a client. func (t *Transport) SecureOutbound(ctx context.Context, insecure net.Conn, p peer.ID) (cs.Conn, error) { cl := tls.Client(insecure, t.identity.ConfigForPeer(p)) - // TODO: use the ctx - // see https://github.com/golang/go/issues/18482 + + // There's no way to pass a context to tls.Conn.Handshake(). + // See https://github.com/golang/go/issues/18482. + // Close the connection instead. + done := make(chan struct{}) + defer close(done) + go func() { + select { + case <-done: + case <-ctx.Done(): + insecure.Close() + } + }() + if err := cl.Handshake(); err != nil { return nil, err } diff --git a/p2p/security/tls/transport_test.go b/p2p/security/tls/transport_test.go index 5e50868c..94b61ea3 100644 --- a/p2p/security/tls/transport_test.go +++ b/p2p/security/tls/transport_test.go @@ -96,6 +96,46 @@ var _ = Describe("Transport", func() { Expect(string(b)).To(Equal("foobar")) }) + It("fails when the context of the outgoing connection is canceled", func() { + clientTransport, err := New(clientKey) + Expect(err).ToNot(HaveOccurred()) + serverTransport, err := New(serverKey) + Expect(err).ToNot(HaveOccurred()) + + clientInsecureConn, serverInsecureConn := connect() + + go func() { + defer GinkgoRecover() + _, err := serverTransport.SecureInbound(context.Background(), serverInsecureConn) + Expect(err).To(HaveOccurred()) + }() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err = clientTransport.SecureOutbound(ctx, clientInsecureConn, serverID) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("use of closed network connection")) + }) + + It("fails when the context of the incoming connection is canceled", func() { + clientTransport, err := New(clientKey) + Expect(err).ToNot(HaveOccurred()) + serverTransport, err := New(serverKey) + Expect(err).ToNot(HaveOccurred()) + + clientInsecureConn, serverInsecureConn := connect() + + go func() { + defer GinkgoRecover() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err := serverTransport.SecureInbound(ctx, serverInsecureConn) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("use of closed network connection")) + }() + _, err = clientTransport.SecureOutbound(context.Background(), clientInsecureConn, serverID) + Expect(err).To(HaveOccurred()) + }) + It("fails if the peer ID doesn't match", func() { thirdPartyID, _ := createPeer() From 1c0f10c904dd928dc069cccd434e8dbaf8011a61 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 29 Nov 2018 09:36:22 +0700 Subject: [PATCH 2/3] return the context cancelation error --- p2p/security/tls/transport.go | 44 ++++++++++++++++-------------- p2p/security/tls/transport_test.go | 6 ++-- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/p2p/security/tls/transport.go b/p2p/security/tls/transport.go index 40181f9a..6bd5ba44 100644 --- a/p2p/security/tls/transport.go +++ b/p2p/security/tls/transport.go @@ -43,30 +43,23 @@ var _ cs.Transport = &Transport{} // SecureInbound runs the TLS handshake as a server. func (t *Transport) SecureInbound(ctx context.Context, insecure net.Conn) (cs.Conn, error) { serv := tls.Server(insecure, t.identity.Config) - - // There's no way to pass a context to tls.Conn.Handshake(). - // See https://github.com/golang/go/issues/18482. - // Close the connection instead. - done := make(chan struct{}) - defer close(done) - go func() { - select { - case <-done: - case <-ctx.Done(): - insecure.Close() - } - }() - - if err := serv.Handshake(); err != nil { - return nil, err - } - return t.setupConn(serv) + return t.handshake(ctx, insecure, serv) } // SecureOutbound runs the TLS handshake as a client. func (t *Transport) SecureOutbound(ctx context.Context, insecure net.Conn, p peer.ID) (cs.Conn, error) { cl := tls.Client(insecure, t.identity.ConfigForPeer(p)) + return t.handshake(ctx, insecure, cl) +} +func (t *Transport) handshake( + ctx context.Context, + // in Go 1.10, we need to close the underlying net.Conn + // in Go 1.11 this was fixed, and tls.Conn.Close() works as well + insecure net.Conn, + tlsConn *tls.Conn, +) (cs.Conn, error) { + errChan := make(chan error, 2) // There's no way to pass a context to tls.Conn.Handshake(). // See https://github.com/golang/go/issues/18482. // Close the connection instead. @@ -76,14 +69,23 @@ func (t *Transport) SecureOutbound(ctx context.Context, insecure net.Conn, p pee select { case <-done: case <-ctx.Done(): + errChan <- ctx.Err() insecure.Close() } }() - if err := cl.Handshake(); err != nil { - return nil, err + if err := tlsConn.Handshake(); err != nil { + // if the context was canceled, return the context error + errChan <- err + return nil, <-errChan } - return t.setupConn(cl) + conn, err := t.setupConn(tlsConn) + if err != nil { + // if the context was canceled, return the context error + errChan <- err + return nil, <-errChan + } + return conn, nil } func (t *Transport) setupConn(tlsConn *tls.Conn) (cs.Conn, error) { diff --git a/p2p/security/tls/transport_test.go b/p2p/security/tls/transport_test.go index 94b61ea3..bc4d5b38 100644 --- a/p2p/security/tls/transport_test.go +++ b/p2p/security/tls/transport_test.go @@ -112,8 +112,7 @@ var _ = Describe("Transport", func() { ctx, cancel := context.WithCancel(context.Background()) cancel() _, err = clientTransport.SecureOutbound(ctx, clientInsecureConn, serverID) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("use of closed network connection")) + Expect(err).To(MatchError(context.Canceled)) }) It("fails when the context of the incoming connection is canceled", func() { @@ -129,8 +128,7 @@ var _ = Describe("Transport", func() { ctx, cancel := context.WithCancel(context.Background()) cancel() _, err := serverTransport.SecureInbound(ctx, serverInsecureConn) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("use of closed network connection")) + Expect(err).To(MatchError(context.Canceled)) }() _, err = clientTransport.SecureOutbound(context.Background(), clientInsecureConn, serverID) Expect(err).To(HaveOccurred()) From eaf15fd98bfcbc788b0508c6ec8dcac8e1827c7e Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 29 Nov 2018 20:38:57 +0700 Subject: [PATCH 3/3] simplify returning of context cancellation errors --- p2p/security/tls/transport.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/p2p/security/tls/transport.go b/p2p/security/tls/transport.go index 6bd5ba44..5a75296f 100644 --- a/p2p/security/tls/transport.go +++ b/p2p/security/tls/transport.go @@ -59,7 +59,6 @@ func (t *Transport) handshake( insecure net.Conn, tlsConn *tls.Conn, ) (cs.Conn, error) { - errChan := make(chan error, 2) // There's no way to pass a context to tls.Conn.Handshake(). // See https://github.com/golang/go/issues/18482. // Close the connection instead. @@ -69,21 +68,24 @@ func (t *Transport) handshake( select { case <-done: case <-ctx.Done(): - errChan <- ctx.Err() insecure.Close() } }() if err := tlsConn.Handshake(); err != nil { // if the context was canceled, return the context error - errChan <- err - return nil, <-errChan + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + return nil, err } conn, err := t.setupConn(tlsConn) if err != nil { // if the context was canceled, return the context error - errChan <- err - return nil, <-errChan + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + return nil, err } return conn, nil }