Skip to content

Commit

Permalink
gbn+mailbox: force connection status change on FIN
Browse files Browse the repository at this point in the history
When the client receives+processes a FIN packet, we force the connection
to status ClientStatusSessionNotFound, as in rare occasions the
corresponding server may have time to close the connection before we've
already processed the sent FIN packet by the server. In that case, if we
didn't force a new status, the client would never mark the connection as
status ClientStatusSessionNotFound.

This solves a bug for the testServerReconnect itest, as that test would
sometimes fail due to the FIN packet being processed but without
changing the connection status, leading to a test predicate never being
satisfied.
  • Loading branch information
ViktorTigerstrom committed Jan 8, 2024
1 parent 77d0bd5 commit 5e16121
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 8 deletions.
4 changes: 4 additions & 0 deletions gbn/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ type config struct {
// packet.
sendToStream sendBytesFunc

// onFIN is a callback that if set, will be called once a FIN packet has
// been received and processed.
onFIN func()

// handshakeTimeout is the time after which the server or client
// will abort and restart the handshake if the expected response is
// not received from the peer.
Expand Down
4 changes: 4 additions & 0 deletions gbn/gbn_conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,10 @@ func (g *GoBackNConn) receivePacketsForever() error { // nolint:gocyclo

close(g.remoteClosed)

if g.cfg.onFIN != nil {
g.cfg.onFIN()
}

return errTransportClosing

default:
Expand Down
8 changes: 8 additions & 0 deletions gbn/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,11 @@ func WithKeepalivePing(ping, pong time.Duration) Option {
conn.pongTime = pong
}
}

// WithOnFIN is used to set the onFIN callback that will be called once a FIN
// packet has been received and processed.
func WithOnFIN(fn func()) Option {
return func(conn *config) {
conn.onFIN = fn
}
}
28 changes: 20 additions & 8 deletions mailbox/client_conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,20 +157,32 @@ func NewClientConn(ctx context.Context, sid [64]byte, serverHost string,
}

c := &ClientConn{
transport: transport,
gbnOptions: []gbn.Option{
gbn.WithTimeout(gbnTimeout),
gbn.WithHandshakeTimeout(gbnHandshakeTimeout),
gbn.WithKeepalivePing(
gbnClientPingTimeout, gbnPongTimeout,
),
},
transport: transport,
status: ClientStatusNotConnected,
onNewStatus: onNewStatus,
quit: make(chan struct{}),
cancel: cancel,
log: logger,
}

c.gbnOptions = []gbn.Option{
gbn.WithTimeout(gbnTimeout),
gbn.WithHandshakeTimeout(gbnHandshakeTimeout),
gbn.WithKeepalivePing(
gbnClientPingTimeout, gbnPongTimeout,
),
gbn.WithOnFIN(func() {
// We force the connection to set a new status after
// processing a FIN packet, as in rare occasions the
// corresponding server may have time to close the
// connection before we've already processed the sent
// FIN packet by the server. In that case, if we didn't
// force a new status, the client would never mark the
// connection as status ClientStatusSessionNotFound.
c.setStatus(ClientStatusSessionNotFound)
}),
}

c.connKit = &connKit{
ctx: ctxc,
impl: c,
Expand Down

0 comments on commit 5e16121

Please sign in to comment.