Wrap outbound client thread stream exceptions #210
Merged
Conversation
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
Unlike the server inbound/outbound and client outbound threads, `http2` is aware of the client outbound thread. If the server disconnects, there is a race between the `http2`-thrown exception and the exception that will come from `grapesy` attempting to read. No matter who wins that race, we want to mark the exception with `ServerDisconnected`.
FinleyMcIlwaine
force-pushed
the
finley/102
branch
2 times, most recently
from
a49fdfa
to
11abc7e
Compare
See module headers of `Test.Sanity.Disconnect` and `Test.Sanity.Exception`
FinleyMcIlwaine
force-pushed
the
finley/102
branch
from
11abc7e
to
5834d3d
Compare
edsko
requested changes
Aug 8, 2024
The 'ReconnectAfter' constructor of reconnect policy now holds an optional 'Server' argument, allowing reconnect policies to specify new server addresses to attempt reconnection to. This makes it possible to fall back to redundant servers, without needing to completely throw away a connection on the client.
We had a few spots where we were defining a `RawRPC "trivial" "trivial"` RPC with `NoMetadata`, so just abstracted to deduplicate.
`rawTestServer` was doing nothing special anymore, and `forkServer` allows us to query the server port as well. We keep `respondWith` around because it is a useful abstraction for modeling broken servers.
No more of that "predicate" nonsense. Client steps are now described by a little DSL and interpreted by the client. Also enable `-Wmissing-export-lists`
FinleyMcIlwaine
force-pushed
the
finley/102
branch
from
ae6159d
to
592ec92
Compare
edsko
reviewed
Aug 29, 2024
| loop :: ReconnectPolicy -> IO () | ||
| loop remainingReconnectPolicy = do | ||
| loop :: Server -> ReconnectPolicy -> IO () | ||
| loop server remainingReconnectPolicy = do |
There was a problem hiding this comment.
Glad to see that the actual implementation was as clean as this :)
| -------------------------------------------------------------------------------} | ||
|
|
||
| foreign import ccall unsafe "kill" c_kill :: CInt -> CInt -> IO () | ||
| foreign import ccall unsafe "exit" c_exit :: CInt -> IO () |
There was a problem hiding this comment.
Ah, I see, fair enough. Then let's add least add a comment here, because exiting the Haskell process this way also doesn't allow the RTS to shutdown cleanly. Of course, that's kind of the point here: this is modelling the server crashing for some reason.
| -- | ||
| -- We shouldn't need to handle this case, since our client never | ||
| -- explicitly sends 'NoMoreElems'. However, see discussion in the | ||
| -- ticket above. |
There was a problem hiding this comment.
Once the http2 stuff is merged, this ticket can be closed right?
There was a problem hiding this comment.
We can do that independently from this PR, of course. Remind me, do we rely on any other recent http2/http-semantics changes for this PR to go through?
edsko
approved these changes
Aug 29, 2024
Reconnect policies can now specify whether they want to attempt reconnection with the original server given to `withConnection`, the last server we attempted connection with, or a new server specified by the policy itself.
FinleyMcIlwaine
force-pushed
the
finley/102
branch
from
a9a191c
to
bab1fdd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Unlike the server inbound/outbound and client outbound threads,
http2is aware of the client outbound thread. If the server disconnects, there is a race between thehttp2-thrown exception and the exception that will come fromgrapesyattempting to read. No matter who wins that race, we want to mark the exception withServerDisconnected.Also add sanity check tests for handling of exceptions/disconnects of client/server while there are open connections with ongoing concurrent calls.
Resolves #102