HPCC-19323 Poll() to handle EINTR consistently #10973
Conversation
|
https://track.hpccsystems.com/browse/HPCC-19323 |
|
@jakesmith please review. |
mckellyln
force-pushed
the
poll_eintr
branch
3 times, most recently
from
38ad577
to
dbc766b
Compare
| @@ -1279,7 +1279,12 @@ bool CSocket::connect_timeout( unsigned timeout, bool noexception) | |||
| else if (rc<0) | |||
| { | |||
| err = ERRNO(); | |||
| LOGERR2(err,2,"::select/poll"); | |||
| if (err != JSE_INTR) | |||
There was a problem hiding this comment.
Would it useful if EINTR event is logged as well? Do we expect them/often?
| @@ -1413,8 +1418,11 @@ void CSocket::connect_wait(unsigned timems) | |||
| if (rc<0) | |||
| { | |||
| err = ERRNO(); | |||
| LOGERR2(err,2,"::select/poll"); | |||
| break; | |||
| if (err != JSE_INTR) | |||
There was a problem hiding this comment.
Would it useful if EINTR event is logged as well? Do we expect them/often?
There was a problem hiding this comment.
I would say generally EINTR events are not worth logging as the code should handle them properly - but then there are some other places in the code where EINTR is logged.
system/jlib/jsocket.cpp
Outdated
| sock->errclose(); | ||
| isopen = false; | ||
| if (!oneshot) | ||
| connectimedout = true; // to force same behavior as before |
There was a problem hiding this comment.
I may be missing something.. how is this same behaviour as before?
There was a problem hiding this comment.
Before it would loop until timed out and then connectimedout would be true. If we break out here early then set it to true to have same return behaviour.
There was a problem hiding this comment.
I could understand the change I think if it was:
if (oneshot)
connecttimedout = true;
but with oneshot == false, it's supposed to loop and retry pre_connect afaics.
system/jlib/jsocket.cpp
Outdated
| isopen = false; | ||
| if (!oneshot) | ||
| connectimedout = true; // to force same behavior as before | ||
| break; |
There was a problem hiding this comment.
Is it right to unconditional break out here?
The other if oneshot is false code looks like it's designed to handle the error and try again (as long as within the timeout period).
There was a problem hiding this comment.
Debatable, but we poll()ed for timeout duration and if poll() failed with something other than EINTR then hard to see why to keep trying until timeout.
There was a problem hiding this comment.
right, there's no way ::select might have exited before the timeout given to it?
And if not, isn't this just as true (if not more so) when oneshot==true, i.e. shouldn't it set connectimedout = true above either way?
I think the comment '// to force same behavior as before' should be removed or change. It won't be helpful to anyone reading the code. Could instead comment why that is' existing because it must be here because waited 'timeout' in ::select above.
|
@mckellyln - a couple of questions. |
|
@jakesmith thanks, I replied with some answers/comments. |
|
@mckellyln - forgot to tag. |
|
@jakesmith rebased to latest master and removed the comment (// to force same behavior) |
|
@mckellyln - it looks fine, can't see any issues. @richardkchapman - ready to merge. |
|
@mckellyln Please squash |
Signed-off-by: Mark Kelly <mark.kelly@lexisnexisrisk.com>
|
@richardkchapman rebased and squashed |
|
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK
|
Add EINTR handling in a few places where poll() could return < 0
Signed-off-by: Mark Kelly mark.kelly@lexisnexisrisk.com
Type of change:
Checklist:
Testing:
Full QA suite passes as expected