soheilhy / cmux

Connection multiplexer for GoLang: serve different services on the same port!
Apache License 2.0
2.57k stars 196 forks source link

Fix index out of range in patricia tree #33

Closed soheilhy closed 8 years ago

soheilhy commented 8 years ago

Bug #32 reported that there is an index out of range error. This issue was introduced in 703b087.

tamird commented 8 years ago

No test?

soheilhy commented 8 years ago

👍 will add the tests. Just want to make sure the patch fixes the bug first.

kalbasit commented 8 years ago

works 👍

kalbasit commented 8 years ago

Although I am getting this:

2016/07/15 16:21:15 http: TLS handshake error from 10.2.64.1:41856: EOF

I'll have to dig more into it, currently on the way to the office, will check in 1/2 an hour.

soheilhy commented 8 years ago

Thanks for the confirmation!

@tamird, added the tests broken on master and fixed on this branch. ptal.

soheilhy commented 8 years ago

RE: EOF

 2016/07/15 16:21:15 http: TLS handshake error from 10.2.64.1:41856: EOF

I beleive this is because you're using an http server with a tls config. Instead, we need to create a tls.NewListener and then passing it to an HTTP server with no tls config. example_tls_test.go is a good example.

kalbasit commented 8 years ago

@soheilhy that wouldn't work the grpc-gateway :(

diff --git a/cmd/serve.go b/cmd/serve.go
index 381c7ce..9e61979 100644
--- a/cmd/serve.go
+++ b/cmd/serve.go
@@ -57,6 +57,7 @@ var (
        loggerService   *logger.Service
        serverClosed    chan bool
        readyToShutdown chan bool
+       tlsConfig       *tls.Config
 )

 // serveCmd represents the serve command
@@ -263,9 +264,10 @@ func serve(cmd *cobra.Command, args []string) {
                // create the HTTP server
                mainServer = &http.Server{
                        Handler: grpcHandlerFunc(gRPCServer, mux),
-                       TLSConfig: &tls.Config{
-                               Certificates: []tls.Certificate{keyPair},
-                       },
+               }
+               // create the TLS config
+               tlsConfig = &tls.Config{
+                       Certificates: []tls.Certificate{keyPair},
                }
        }

@@ -357,10 +359,8 @@ func serve(cmd *cobra.Command, args []string) {
                                }
                                // wait for the server to die
                                <-serverClosed
-                               // create a new HTTP server
-                               mainServer.TLSConfig = &tls.Config{
-                                       Certificates: []tls.Certificate{keyPair},
-                               }
+                               // update the certificate in tlsConfig
+                               tlsConfig.Certificates = []tls.Certificate{keyPair}
                                // create a new connection
                                if srvListener, err = net.Listen("tcp", listenAddr); err != nil {
                                        log.Fatalf("error opening a socket on %s: %s", listenAddr, err)
@@ -379,12 +379,12 @@ func startServer(mainServer, healthCheckServer *http.Server, srvListener net.Lis
        log.Printf("starting the HTTP/HTTPS/gRPC server bound to %q", listenAddr)
        // create a new connection multiplexer.
        m := cmux.New(srvListener)
-       // we first match on HTTP 1.1 methods.
-       httpl := m.Match(cmux.HTTP1Fast())
+       // we first match on the ReadinessProbe header
+       httpl := m.Match(cmux.HTTP1HeaderField("ReadinessProbe", "32b62d7a2c4227fbf239a9ebe7bce8b9a32cf069"))
        // if not matched, we assume that its TLS.
        tlsl := m.Match(cmux.Any())
        // start the mainServer
-       go mainServer.Serve(tls.NewListener(tlsl, mainServer.TLSConfig))
+       go mainServer.Serve(tls.NewListener(tlsl, tlsConfig))
        // start the healthCheckServer
        go healthCheckServer.Serve(httpl)
        // boot the connection multiplexer, this should return once the srvListener

when I run it:

2016/07/15 10:34:27 starting the HTTP/HTTPS/gRPC server bound to ":8091"
2016/07/15 10:34:27 transport: http2Client.notifyError got notified that the client transport was broken unexpected EOF.
2016/07/15 10:34:27 transport: http2Client.notifyError got notified that the client transport was broken unexpected EOF.
2016/07/15 10:34:29 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: write tcp 127.0.0.1:52766->127.0.0.1:8091: write: broken pipe"; Reconnecting to {"127.0.0.1:8091" <nil>}
soheilhy commented 8 years ago

@kalbasit hmm... I don't have a good answer on why TLS doesn't work for you. I tried a few examples and seems to be working fine. Since this isn't related to this pull request, can you please create another issue with a minimal example to reproduce the bug?

Thanks.

kalbasit commented 8 years ago

@soheilhy agree, I'll investigate more and if I'll make an minimal example if needed.

tamird commented 8 years ago

lgtm

soheilhy commented 8 years ago

Thanks guys!