-
Notifications
You must be signed in to change notification settings - Fork 24
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
add support for sending error codes on session close #121
Open
sukunrt
wants to merge
12
commits into
master
Choose a base branch
from
sukun/conn-error-2
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
e7338b0
introduce GoAwayError type
sukunrt d8cf4e7
send GoAway on Close
sukunrt 4b262c0
add CloseWithError
sukunrt ea5605b
move errors back to const
sukunrt 8adb9a8
fix race in write timeout
sukunrt f56b1c3
add support for sending error codes on stream reset
sukunrt 9190b78
fix err on conn close
sukunrt 5727def
only send goaway on close
sukunrt 43cd707
Merge branch 'sukun/stream-error-code' into sukun/conn-error-2
sukunrt ede18a5
don't block on connection close
sukunrt 3eaea39
review comments
sukunrt 39abe7e
use ErrStreamReset for resetting streams
sukunrt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,10 +46,6 @@ var nullMemoryManager = &nullMemoryManagerImpl{} | |
type Session struct { | ||
rtt int64 // to be accessed atomically, in nanoseconds | ||
|
||
// remoteGoAway indicates the remote side does | ||
// not want futher connections. Must be first for alignment. | ||
remoteGoAway int32 | ||
|
||
// localGoAway indicates that we should stop | ||
// accepting futher connections. Must be first for alignment. | ||
localGoAway int32 | ||
|
@@ -102,6 +98,8 @@ type Session struct { | |
// recvDoneCh is closed when recv() exits to avoid a race | ||
// between stream registration and stream shutdown | ||
recvDoneCh chan struct{} | ||
// recvErr is the error the receive loop ended with | ||
recvErr error | ||
|
||
// sendDoneCh is closed when send() exits to avoid a race | ||
// between returning from a Stream.Write and exiting from the send loop | ||
|
@@ -203,9 +201,6 @@ func (s *Session) OpenStream(ctx context.Context) (*Stream, error) { | |
if s.IsClosed() { | ||
return nil, s.shutdownErr | ||
} | ||
if atomic.LoadInt32(&s.remoteGoAway) == 1 { | ||
return nil, ErrRemoteGoAway | ||
} | ||
|
||
// Block if we have too many inflight SYNs | ||
select { | ||
|
@@ -283,9 +278,23 @@ func (s *Session) AcceptStream() (*Stream, error) { | |
} | ||
} | ||
|
||
// Close is used to close the session and all streams. | ||
// Attempts to send a GoAway before closing the connection. | ||
// Close is used to close the session and all streams. It doesn't send a GoAway before | ||
// closing the connection. | ||
func (s *Session) Close() error { | ||
return s.close(ErrSessionShutdown, false, goAwayNormal) | ||
} | ||
|
||
// CloseWithError is used to close the session and all streams after sending a GoAway message with errCode. | ||
// Blocks for ConnectionWriteTimeout to write the GoAway message. | ||
// | ||
// The GoAway may not actually be sent depending on the semantics of the underlying net.Conn. | ||
// For TCP connections, it may be dropped depending on LINGER value or if there's unread data in the kernel | ||
// receive buffer. | ||
func (s *Session) CloseWithError(errCode uint32) error { | ||
return s.close(&GoAwayError{Remote: false, ErrorCode: errCode}, true, errCode) | ||
} | ||
|
||
func (s *Session) close(shutdownErr error, sendGoAway bool, errCode uint32) error { | ||
s.shutdownLock.Lock() | ||
defer s.shutdownLock.Unlock() | ||
|
||
|
@@ -294,35 +303,42 @@ func (s *Session) Close() error { | |
} | ||
s.shutdown = true | ||
if s.shutdownErr == nil { | ||
s.shutdownErr = ErrSessionShutdown | ||
s.shutdownErr = shutdownErr | ||
} | ||
close(s.shutdownCh) | ||
s.conn.Close() | ||
s.stopKeepalive() | ||
<-s.recvDoneCh | ||
|
||
// Only send GoAway if we have an error code. | ||
if sendGoAway && errCode != goAwayNormal { | ||
// wait for write loop to exit | ||
// We need to write the current frame completely before sending a goaway. | ||
// This will wait for at most s.config.ConnectionWriteTimeout | ||
<-s.sendDoneCh | ||
ga := s.goAway(errCode) | ||
if err := s.conn.SetWriteDeadline(time.Now().Add(goAwayWaitTime)); err == nil { | ||
_, _ = s.conn.Write(ga[:]) // there's nothing we can do on error here | ||
} | ||
s.conn.SetWriteDeadline(time.Time{}) | ||
} | ||
|
||
s.conn.Close() | ||
<-s.sendDoneCh | ||
<-s.recvDoneCh | ||
|
||
resetErr := shutdownErr | ||
if _, ok := resetErr.(*GoAwayError); !ok { | ||
resetErr = fmt.Errorf("%w: connection closed: %w", ErrStreamReset, shutdownErr) | ||
} | ||
s.streamLock.Lock() | ||
defer s.streamLock.Unlock() | ||
for id, stream := range s.streams { | ||
stream.forceClose() | ||
stream.forceClose(resetErr) | ||
delete(s.streams, id) | ||
stream.memorySpan.Done() | ||
} | ||
return nil | ||
} | ||
|
||
// exitErr is used to handle an error that is causing the | ||
// session to terminate. | ||
func (s *Session) exitErr(err error) { | ||
s.shutdownLock.Lock() | ||
if s.shutdownErr == nil { | ||
s.shutdownErr = err | ||
} | ||
s.shutdownLock.Unlock() | ||
s.Close() | ||
} | ||
|
||
// GoAway can be used to prevent accepting further | ||
// connections. It does not close the underlying conn. | ||
func (s *Session) GoAway() error { | ||
|
@@ -451,7 +467,7 @@ func (s *Session) startKeepalive() { | |
|
||
if err != nil { | ||
s.logger.Printf("[ERR] yamux: keepalive failed: %v", err) | ||
s.exitErr(ErrKeepAliveTimeout) | ||
s.close(ErrKeepAliveTimeout, false, 0) | ||
} | ||
}) | ||
} | ||
|
@@ -516,7 +532,25 @@ func (s *Session) sendMsg(hdr header, body []byte, deadline <-chan struct{}) err | |
// send is a long running goroutine that sends data | ||
func (s *Session) send() { | ||
if err := s.sendLoop(); err != nil { | ||
s.exitErr(err) | ||
// If we are shutting down because remote closed the connection, prefer the recvLoop error | ||
// over the sendLoop error. The receive loop might have error code received in a GoAway frame, | ||
// which was received just before the TCP RST that closed the sendLoop. | ||
// | ||
// If we are closing because of an write error, we use the error from the sendLoop and not the recvLoop. | ||
// We hold the shutdownLock, close the connection, and wait for the receive loop to finish and | ||
// use the sendLoop error. Holding the shutdownLock ensures that the recvLoop doesn't trigger connection close | ||
// but the sendLoop does. | ||
s.shutdownLock.Lock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment as to why you are holding the shutdownLock around this section. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added some comment, can you review once more? |
||
if s.shutdownErr == nil { | ||
s.conn.Close() | ||
<-s.recvDoneCh | ||
if _, ok := s.recvErr.(*GoAwayError); ok { | ||
err = s.recvErr | ||
} | ||
s.shutdownErr = err | ||
} | ||
s.shutdownLock.Unlock() | ||
s.close(err, false, 0) | ||
} | ||
} | ||
|
||
|
@@ -644,7 +678,7 @@ func (s *Session) sendLoop() (err error) { | |
// recv is a long running goroutine that accepts new data | ||
func (s *Session) recv() { | ||
if err := s.recvLoop(); err != nil { | ||
s.exitErr(err) | ||
s.close(err, false, 0) | ||
} | ||
} | ||
|
||
|
@@ -666,7 +700,10 @@ func (s *Session) recvLoop() (err error) { | |
err = fmt.Errorf("panic in yamux receive loop: %s", rerr) | ||
} | ||
}() | ||
defer close(s.recvDoneCh) | ||
defer func() { | ||
s.recvErr = err | ||
close(s.recvDoneCh) | ||
}() | ||
var hdr header | ||
for { | ||
// fmt.Printf("ReadFull from %#v\n", s.reader) | ||
|
@@ -781,18 +818,15 @@ func (s *Session) handleGoAway(hdr header) error { | |
code := hdr.Length() | ||
switch code { | ||
case goAwayNormal: | ||
atomic.SwapInt32(&s.remoteGoAway, 1) | ||
return ErrRemoteGoAway | ||
case goAwayProtoErr: | ||
s.logger.Printf("[ERR] yamux: received protocol error go away") | ||
return fmt.Errorf("yamux protocol error") | ||
case goAwayInternalErr: | ||
s.logger.Printf("[ERR] yamux: received internal error go away") | ||
return fmt.Errorf("remote yamux internal error") | ||
default: | ||
s.logger.Printf("[ERR] yamux: received unexpected go away") | ||
return fmt.Errorf("unexpected go away received") | ||
s.logger.Printf("[ERR] yamux: received go away with error code: %d", code) | ||
} | ||
return nil | ||
return &GoAwayError{Remote: true, ErrorCode: code} | ||
} | ||
|
||
// incomingStream is used to create a new incoming stream | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we updated the connection manager to be able to deal with potential blocking here? Also, we should probably document it.