Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

session.go: AcceptStream handles GoAway #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions session.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ type Session struct {
// acceptCh is used to pass ready streams to the client
acceptCh chan *Stream

// goAwayCh is used to notify AcceptStream of GoAway requests
goAwayCh chan struct{}

// sendCh is used to mark a stream as ready to send,
// or to send a header out directly.
sendCh chan sendReady
Expand Down Expand Up @@ -92,6 +95,7 @@ func newSession(config *Config, conn io.ReadWriteCloser, client bool) *Session {
streams: make(map[uint32]*Stream),
synCh: make(chan struct{}, config.AcceptBacklog),
acceptCh: make(chan *Stream, config.AcceptBacklog),
goAwayCh: make(chan struct{}, 1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little bit of a formatting fail (needs to be indented). Also, make(chan struct{}) should be sufficient.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should create the goAwayCh only if the peer is a server so I've moved it to the if client {}else{}. Let me know if you think otherwise.
I've noticed that now session_test.go:375 - client2.GoAway() always throws err: session shutdown. It shouldn't be an issue as the client shouldn't try to open connections after GoAway but still I don't know why it errors out.

sendCh: make(chan sendReady, 64),
recvDoneCh: make(chan struct{}),
shutdownCh: make(chan struct{}),
Expand Down Expand Up @@ -188,6 +192,9 @@ func (s *Session) Accept() (net.Conn, error) {
// AcceptStream is used to block until the next available stream
// is ready to be accepted.
func (s *Session) AcceptStream() (*Stream, error) {
if atomic.LoadInt32(&s.remoteGoAway) == 1 {
return nil, ErrRemoteGoAway
}
select {
case stream := <-s.acceptCh:
if err := stream.sendWindowUpdate(); err != nil {
Expand All @@ -196,6 +203,8 @@ func (s *Session) AcceptStream() (*Stream, error) {
return stream, nil
case <-s.shutdownCh:
return nil, s.shutdownErr
case <- s.goAwayCh:
return nil, ErrRemoteGoAway
}
}

Expand Down Expand Up @@ -416,6 +425,10 @@ func (s *Session) recvLoop() error {
handler = s.handleStreamMessage
case typeGoAway:
handler = s.handleGoAway
select{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this in to handleGoAway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

case s.goAwayCh <- struct{}{}:
default:
}
case typePing:
handler = s.handlePing
default:
Expand Down
43 changes: 43 additions & 0 deletions session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,49 @@ func TestGoAway(t *testing.T) {
}
}

func TestGoAwayClient(t *testing.T) {
client, server := testClientServer()
defer client.Close()
defer server.Close()
done := make(chan struct{}, 1)
go func(){
if err := client.GoAway(); err != nil {
t.Fatalf("err: %v", err)
}
close(done)
}()
<- done
_, err := server.Accept()
if err != ErrRemoteGoAway {
t.Errorf("err: %v", err)
}
// Test GoAway while Accept is running.
client2, server2 := testClientServer()
defer client2.Close()
defer server2.Close()
done = make(chan struct{}, 1)
go func(){
<- done
time.Sleep(500 *time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spacing is strange - I'd probably just run go fmt on this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

if err := client2.GoAway(); err != nil {
t.Fatalf("err: %v", err)
}
}()
errCh := make(chan error, 1)
go func(){
close(done)
_, err := server2.Accept()
errCh <- err
}()
select{
case err = <- errCh:
if err != ErrRemoteGoAway {
t.Errorf("err: %v", err)
}
case <- time.After(2 *time.Second):
t.Errorf("Timeout awaiting ErrRemoteGoAway")
}
}
func TestManyStreams(t *testing.T) {
client, server := testClientServer()
defer client.Close()
Expand Down