calling conn close before stream close results in a soft deadlock

[erikh]

        conn, dialErr := net.Dial("tcp", LISTEN_ADDRESS)
        if dialErr != nil {
            b.Fatalf("Error dialing server: %s", dialErr)
        }

        spdyConn, spdyErr := NewConnection(conn, false)
        if spdyErr != nil {
            b.Fatalf("Error creating spdy connection: %s", spdyErr)
        }

        go spdyConn.Serve(MirrorStreamHandler)

        stream, err := spdyConn.CreateStream(http.Header{}, nil, false)

        writer := make([]byte, 1024)

        stream.Write(writer)

        if err != nil {
            panic(err)
        }

        reader := make([]byte, 1024)
        stream.Read(reader)

        closeErr := spdyConn.Close()
        if closeErr != nil {
            b.Fatalf("Error closing connection: %s, closeErr")
        }

This code should soft deadlock, or at least, that's what I was seeing in the runs I made.

if you add a stream.Close() after the stream.Read() call, it exhibits the desired behavior.

[erikh]

It looks like this is a feature, as calling SetCloseTimeout() should fix the issue.

I'm not sure this is the best way to approach it. I call Close() on the connection, and I want the whole thing to tear down, not block waiting for streams I've never closed to be torn down. I don't believe it's a good idea to keep close blocking.

Perhaps exposing the waitClose() method would give people a better option to wait for a close after Close() is called, which will allow the user to care or not, or have WaitClose() do the closing itself.

[dmcgowan]

I don't think it is unreasonable to set a default timeout for closing, I just have no idea what it should be. As for tearing down, Close should initiate the teardown and the spdy spec defines how that process goes. It is also not unreasonable to have a ForceClose to hard shutdown the connection, but that if that is the desired behavior then close can be called on the underlying transport. IMO the Close function should have the GOAWAY behavior as defined by spdy, which does not close immediately, it sends a GOAWAY frame and only closes the transport when the streams are closed (or a reasonable amount of time for the other end to clean up). Only other forms of closing are ungraceful (as if the transport closed).

http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3#TOC-3.1.1-Use-of-GOAWAY

http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3#TOC-2.6.6-GOAWAY

[erikh]

close() in unix (and by axiom, bsd sockets) parlance means “close immediately”. There are few things that can cause it to interrupt, and none that I can think of that block. I think we should stick to that as that’s the behavior the rest of the Close() functions in net.* emulate.

I really want to strongly suggest exposing WaitClose() instead, which gives you the desired functionality without compromising the expectations a network developer is going to have when calling Close().

[dmcgowan]

Close immediately should never be the desired behavior on a non-test session, the correct close behavior should implement the expected golang interface. If this is confusing to developers, then we should ensure it is documented and the underlying spdy spec is understood. Yes this is a higher level library, but spdy can only be abstracted away so much.

[erikh]

So, what you’re suggesting is that because it’s not correct, we should hide the wait method from the user and introduce unique behavior on a common method amongst network toolkits? One that isn’t even consistent with the Stream and net.Conn interfaces?

I don’t think we need to coddle our users, and especially not make it surprising to use the library in a way that is consistent with others. I cannot think of a library offhand that implements close in this manner. Managing connections is the user’s problem and we should empower them to do that in the best way they think appropriate.

I can do this patch, that’s no problem. When I want a connection to close, I shouldn’t have to set the timeout to 1*time.Nanosecond to hack it to exhibit normal socket behavior.

POLS leads to happier developers and less angst around library interfaces.

/cc @shykes

[shykes]

3 suggestions :

1. Can't we use net.Conn.Close() when we want socket-close, and
   spdystream.Session.Close() when we want spdy-level session close?

2. We could look at other spdy implementations and see what they do.

3. We could ask Go maintainers for their opinion (what would they prefer if
   this were to be merged in stdlib?)

On Sunday, July 6, 2014, Erik Hollensbe notifications@github.com wrote:

So, what you’re suggesting is that because it’s not correct, we should
hide the wait method from the user and introduce unique behavior on a
common method amongst network toolkits? One that isn’t even consistent with
the Stream and net.Conn interfaces?

I don’t think we need to coddle our users, and especially not make it
surprising to use the library in a way that is consistent with others. I
cannot think of a library offhand that implements close in this manner.
Managing connections is the user’s problem and we should empower them to do
that in the best way they think appropriate.

I can do this patch, that’s no problem. When I want a connection to close,
I shouldn’t have to set the timeout to 1*time.Nanosecond to hack it to
exhibit normal socket behavior.

POLS leads to happier developers and less angst around library interfaces.

/cc @shykes

—
Reply to this email directly or view it on GitHub
#24 (comment).

[erikh]

@shykes the problem is that I'm pretty sure this library will leak goroutines if net.Conn.Close() is called, because the connection management does not account for the underlying connection to close.

[shykes]

Really? I would expect it to cause Read()s and Write()s to fail, causing
all the goroutines to clean themselves up in a chain.

On Tuesday, July 8, 2014, Erik Hollensbe notifications@github.com wrote:

@shykes https://github.com/shykes the problem is that I'm pretty sure
this library will leak goroutines if net.Conn.Close() is called, because
the connection management does not account for the underlying connection to
close.

—
Reply to this email directly or view it on GitHub
#24 (comment).

[erikh]

It should, yes. I don’t think it does so now — the soft lock that happens when you close the SPDY connection is at fault here. I can ensure one way or another later today.

-Erik

On Jul 8, 2014, at 8:32 AM, Solomon Hykes notifications@github.com wrote:

Really? I would expect it to cause Read()s and Write()s to fail, causing
all the goroutines to clean themselves up in a chain.

On Tuesday, July 8, 2014, Erik Hollensbe notifications@github.com wrote:

@shykes https://github.com/shykes the problem is that I'm pretty sure
this library will leak goroutines if net.Conn.Close() is called, because
the connection management does not account for the underlying connection to
close.

—
Reply to this email directly or view it on GitHub
#24 (comment).

—
Reply to this email directly or view it on GitHub.

[dmcgowan]

I think we are in agreement on the solution. Have close send the GOAWAY and return immediately after, only returning an error on GOAWAY write or before. The close will then background the waiting portion of the close, which will either complete and end or timeout and end. If the timeout occurs and the connection is closed, that will trigger the post server clean up which should clean up all server related go routines. Currently the burden of closing the streams is put on the application, I don't think it would be unreasonable to alter to state of all streams in the stream map in connection closed scenario, I would do this in the post server cleanup before it wipes out the stream map. Either way no spdystream goroutines should outlive the cleanup.