OpenStream hanging on synCh about yamux HOT 7 CLOSED

@whyrusleeping Can you verify that the accept backlog isn't being reached? e.g. it is expected to hang if the backlog is full, but you are saying that it is hanging when the acceptor has not reached the backlog. Do both sides have the same backlog configured (I think that is a potential issue if the two sides disagree).

from yamux.

@armon i'm pretty sure the accept backlog isnt being reached, after reporting the issue i bumped the number up to 8192, which made the issue less likely to occur, but I still encounter it.

I'm reasonably certain that both sides are using the same values for accept backlog. we recently made a breaking change to our network protocol requiring everyone to update to the latest code in order to be on the network, and I still see the hang happening. Here is a stack trace if you are interested:

https://ipfs.io/ipfs/QmbmuLGAf182VeRvQA64CVMkkp8fUsXttKUv8w94oNkQKj (ctrl+f session.go:149 )

from yamux.

Any update here? I hit this issue rather frequently

from yamux.

correction to the situation: We're likely hitting the accept backlog limit, but hitting that shouldnt cause it to permanently hang. It should just act like a semaphore and hang until something else completes

from yamux.

I believe i've found the issue. If you open a stream, write to it, then close it fast enough, you can close it before its 'established' thus causing the synCh semaphore to not be released.

Heres a repro:

package main

import (
    "fmt"
    "net"
    "time"

    "github.com/hashicorp/yamux"
)

func tcppipe() (net.Conn, net.Conn) {
    list, err := net.Listen("tcp", "0.0.0.0:0")
    if err != nil {
        panic(err)
    }

    con, err := net.Dial("tcp", list.Addr().String())
    if err != nil {
        panic(err)
    }

    con2, err := list.Accept()
    if err != nil {
        panic(err)
    }

    return con, con2
}

func main() {
    //a, b := net.Pipe()
    a, b := tcppipe() // the slight latency increase of tcp makes the bug more apparent

    cfg_a := &yamux.Config{
        AcceptBacklog:          16,
        ConnectionWriteTimeout: time.Second * 10,
        EnableKeepAlive:        true,
        MaxStreamWindowSize:    256 << 10,
        KeepAliveInterval:      time.Second * 10,
    }

    cfg_b := &yamux.Config{
        AcceptBacklog:          16,
        ConnectionWriteTimeout: time.Second * 10,
        EnableKeepAlive:        true,
        MaxStreamWindowSize:    256 << 10,
        KeepAliveInterval:      time.Second * 10,
    }

    s, err := yamux.Server(a, cfg_a)
    if err != nil {
        panic(err)
    }
    defer s.Close()

    c, err := yamux.Client(b, cfg_b)
    if err != nil {
        panic(err)
    }
    defer c.Close()

    go func() {
        for {
            ns, err := s.OpenStream()
            if err != nil {
                panic(err)
            }
            ns.Close()
            fmt.Println("opened")
        }
    }()

    for {
        ac, err := c.AcceptStream()
        if err != nil {
            panic(err)
        }
        ac.Close()
    }
}

My current 'patch' is to keep a variable called 'established' on the stream, and if we call closeStream on a stream that belongs to us (id % 2 == session.nextID % 2) and its established field is false, we call establishStream on it to release the token. Its a hack and there is likely a better way to do this that someone more familiar with the codebase can write.

from yamux.

@whyrusleeping Sorry for the delay, this ticket probably got lost in the noise of issues we get. Tagging @slackpad to investigate this.

from yamux.

@armon awesome, thanks :)

from yamux.