Add send/ recv timeouts to message endpoints #119
Merged
Conversation
This file contains hidden or 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
Shillaker
commented
Jun 18, 2021
| int recvTimeoutMs = DEFAULT_RECV_TIMEOUT_MS; | ||
| int sendTimeoutMs = DEFAULT_SEND_TIMEOUT_MS; | ||
|
|
||
| void validateTimeout(int value); |
There was a problem hiding this comment.
These have to be protected because of the subclassing that goes on (otherwise they'd be private).
Shillaker
commented
Jun 18, 2021
|
|
||
| // Print default message and rethrow | ||
| SPDLOG_ERROR("Error receiving message: {} ({})", e.num(), e.what()); | ||
| throw; |
There was a problem hiding this comment.
This message was missing the error code before.
Shillaker
commented
Jun 18, 2021
| server.stop(); | ||
| } | ||
|
|
||
| TEST_CASE("Test client timeout on requests to valid server", "[transport]") |
There was a problem hiding this comment.
Not sure if this is the right place to put this test. It requires a custom server, but is really testing functionality in the client.
csegarragonz
approved these changes
Jun 18, 2021
|
|
||
| void MessageEndpoint::validateTimeout(int value) | ||
| { | ||
| if (value <= 0) { |
There was a problem hiding this comment.
So how would we specify an infinte timeout? Do we want to support that at all? I would expect a negative timeout to not timeout at all.
There was a problem hiding this comment.
We should not support infinite timeouts anywhere. I can't think of a case where it's justified.
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.
At the moment we don't set any timeouts in our use of 0MQ, which means we get an infinite hang if something goes wrong. Instead we would like things to time out, so this PR sets the send timeout and recv timeout on every 0MQ socket by default.
I actually can't work out in which scenario the send timeout would be triggered, hence there's no test for it and it could be overkill. However, for now I'd like to keep it in case it catches any of the issues we've been seeing.