fix: use separate session with timeout in session.done() handler - #491
fix: use separate session with timeout in session.done() handler#491annatar wants to merge 4 commits into
Conversation
|
|
||
| if s.state != nil { | ||
| if err := s.state.ReleaseState(ctx); err != nil { | ||
| releaseContext, cancel := context.WithTimeout(ctx, 5*time.Second) |
There was a problem hiding this comment.
You still inherit the ctx cancellation from the parent even in this case. The DB transaction will and should still fail.
Now why that fixes the underlying flaky test is a different topic.
Plus this impacts prod. We now have exactly 5 secs to release the state.
There was a problem hiding this comment.
I don't see how it resolves the issue
There was a problem hiding this comment.
BTW, this leads to a bigger issue; the user state WG won't be released if the transaction fails first, preventing server shutdown and removing the user from the server.
So we ought to either re-order the operations downstream on releaseState, or perhaps introduce a separate context for this operation.
Either way it's good you picked this up.
There was a problem hiding this comment.
I was getting contextcheck lint errors when using a brand new context, but I guess we don't need to pass the parent context, and I guess a timeout context doesn't make sense, since RemoveState down the patch, calls user.statesWG.Done so when the whole procedure finishes we defer cancel() with the new context.
Although this means that we allow remove state to finish even if the context is cancelled, which I guess should be the case?
124e802 to
740bb8e
Compare
3dab740 to
085b138
Compare
3d948c7 to
cd86397
Compare
085b138 to
3f387d1
Compare
Identified from a test failing on CI. Running this locally with `go test -v -count 7000 -timeout 10m ./tests -run TestNoopReceivesPendingDeletionUpdates` will produce the same issue and The issue being the context passed to `done` is the same one that gets cancelled, which in session `retrun ctx.Err()`, on shutdown removeState's first context gated DB read fails with the context cancelled, the function returns early, `state.Close(ctx)` never runs, thus the DB handle is never released. On macOS/Linux this would not pose an issue, but on Windows `TempDir RemoveAll` runs right after this so the `.db` file is still inuse making the test fail. Added a new releaseContext with a 5s timeout, that is used on RemoveState. Re-running the test multiple times using `count 7000` ~42k times, doesn't fail now.
3f387d1 to
5173834
Compare
cd86397 to
5846824
Compare
2 participants
Identified from a test failing on CI. Running this locally with
go test -v -count 7000 -timeout 10m ./tests -run TestNoopReceivesPendingDeletionUpdateswill produce the same issue andThe issue being the context passed to
doneis the same one that gets cancelled, which in session.serve gets anreturn ctx.Err()which.done(ctx)is called on defer. On the waterfall shutdown removeState's first context gated DB read fails with the context cancelled, the function returns early,state.Close(ctx)never runs, thus the DB handle is never released.On macOS/Linux this would not pose an issue, but on Windows
TempDir RemoveAllruns right after this so the.dbfile is still inuse making the test fail.Added a new releaseContext with a 5s timeout, that is used on RemoveState.
Re-running the test multiple times using
count 7000~42k times, doesn't fail now.