hashicorp / yamux

Golang connection multiplexing library
Mozilla Public License 2.0
2.25k stars 236 forks source link

http.Serve(Session, nil) hangs on abortPendingRead when using Hijack() #58

Closed hagna closed 2 years ago

hagna commented 6 years ago

This minimal example hangs on go 1.10 when I add log.Printf to hijackLocked() (net/http/server.go:300). It hangs on abortPendingRead() (net/http/server.go:692) when I can get it to hang. What are some possible issues with hijacking?

package main

import (
    "context"
    "flag"
    "fmt"
    "github.com/hashicorp/yamux"
    "net"
    "net/http"
    "time"
)

var endpoint = flag.String("ep", "127.0.0.1:8000", "endpoint")

func client() {
    // Get a TCP connection
    conn, err := net.Dial("tcp", *endpoint)
    if err != nil {
        panic(err)
    }

    // Setup client side of yamux
    session, err := yamux.Client(conn, nil)
    if err != nil {
        panic(err)
    }
    http.Serve(session, nil)

}

func server() {
    // Accept a TCP connection
    listener, err := net.Listen("tcp", *endpoint)
    if err != nil {
        panic(err)
    }
    conn, err := listener.Accept()
    if err != nil {
        panic(err)
    }

    // Setup server side of yamux
    session, err := yamux.Server(conn, nil)
    if err != nil {
        panic(err)
    }
    tr := &http.Transport{
        DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
            return session.Open()
        },
    }
    client := &http.Client{Transport: tr}
    resp, err := client.Get("http://127.0.0.1:8000/works")
    if err != nil {
        panic(err)
    }
    fmt.Println(resp)
    fmt.Println("hanging in abortPendingRead()...")
    resp, err = client.Get("http://127.0.0.1:8000/bug")
    if err != nil {
        fmt.Println("we should not see this because it should hang in abortPendingRead()")
        fmt.Println(err)
    }
    fmt.Println(resp)

}

func main() {
    flag.Parse()
    http.HandleFunc("/works", func(w http.ResponseWriter, r *http.Request) {
        fmt.Fprintf(w, "this works")
    })
    http.HandleFunc("/bug", func(w http.ResponseWriter, r *http.Request) {
        h, ok := w.(http.Hijacker)
        if !ok {
            fmt.Printf("does not support hijack")
        }
        c, _, err := h.Hijack()
        if err != nil {
            panic(err)
        }
        fmt.Fprintf(c, "this works")
        c.Close()
    })

    go server()
    time.Sleep(90 * time.Millisecond)
    client()
}
filipochnik commented 6 years ago

Likely related: https://github.com/hashicorp/yamux/pull/51

hagna commented 6 years ago

Thanks. Commit c435ed1 does appear to fix it, and by that I mean this:

@@ -440,12 +440,14 @@ func (s *Stream) SetDeadline(t time.Time) error {
 // SetReadDeadline sets the deadline for future Read calls.
 func (s *Stream) SetReadDeadline(t time.Time) error {
        s.readDeadline.Store(t)
+       asyncNotify(s.recvNotifyCh)
        return nil
 }

 // SetWriteDeadline sets the deadline for future Write calls
 func (s *Stream) SetWriteDeadline(t time.Time) error {
        s.writeDeadline.Store(t)
+       asyncNotify(s.sendNotifyCh)
        return nil
 }