-
Notifications
You must be signed in to change notification settings - Fork 15
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 send/ recv timeouts to message endpoints #119
Conversation
int recvTimeoutMs = DEFAULT_RECV_TIMEOUT_MS; | ||
int sendTimeoutMs = DEFAULT_SEND_TIMEOUT_MS; | ||
|
||
void validateTimeout(int value); |
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.
These have to be protected
because of the subclassing that goes on (otherwise they'd be private).
|
||
// Print default message and rethrow | ||
SPDLOG_ERROR("Error receiving message: {} ({})", e.num(), e.what()); | ||
throw; |
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.
This message was missing the error code before.
@@ -163,4 +186,56 @@ TEST_CASE("Test multiple clients talking to one server", "[transport]") | |||
|
|||
server.stop(); | |||
} | |||
|
|||
TEST_CASE("Test client timeout on requests to valid server", "[transport]") |
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.
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.
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.
LGTM. Just one minor pint re. infinite timeouts.
@@ -253,6 +265,31 @@ int MessageEndpoint::getPort() | |||
return port; | |||
} | |||
|
|||
void MessageEndpoint::validateTimeout(int value) | |||
{ | |||
if (value <= 0) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not support infinite timeouts anywhere. I can't think of a case where it's justified.
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.