logreader/logwriter: fix race condition of idle timer and scheduled I/O job #2650
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
|
Build FAILURE |
gaborznagy
changed the title
|
@kira-syslogng retest this please; |
|
Build SUCCESS |
gaborznagy
force-pushed
the
idle-timer-race
branch
from
579dac4
to
ed7781e
Compare
gaborznagy
changed the title
|
Build SUCCESS |
furiel
previously approved these changes
Apr 2, 2019
MrAnno
previously approved these changes
Apr 2, 2019
|
@alltilla kindly pointed out that we should make LogReader and LogWriter symmetric. I'll add it soon and rerun some additional tests. marking as WIP until. |
gaborznagy
changed the title
gaborznagy
force-pushed
the
idle-timer-race
branch
2 times, most recently
from
85cb7fa
to
e1ee285
Compare
|
Build FAILURE |
|
@kira-syslogng retest this please |
|
Build SUCCESS |
There is a race condition between scheduled I/O operations in worker threads and closing the idle connections (response/ack_timeout timer). The idle timer is not stopped, just when an I/O operation finished and we would like to re-schedule the next action on source side (update_watches()). The timer can trigger a connection closing, although there is schedule I/O operation. Timer should be stopped when we schedule an I/O. This way we eliminate the race condition between 2 sides. We do stop I/O polling (see calling log_reader_stop_watches() in log_reader_io_handle_in(), also log_writer_stop_watches() in log_writer_io_handler()). About stop_idle_timer cals in update_watches: Initially I thought that calling idle_timer is unnecessary in update_watches, since we stop the timer in io_handle. I've ven put an assertion to the timer registration, disabling timer re-registering. I forgot that in case of suspend, we will call an update_watches again, where an idle_timer registration will fail due to the assertion. I've put bach the idle timer stopping. Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>
gaborznagy
force-pushed
the
idle-timer-race
branch
from
e1ee285
to
6432a7c
Compare
|
About stop_idle_timer calls in update_watches: |
|
Build SUCCESS |
added 2 commits
April 3, 2019 14:11
Previous patch eliminates race condition of idle_timer and scheduled IO job. Still adding protection can help in future changes. Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>
Signed-off-by: Gabor Nagy <gabor.nagy@balabit.com>
gaborznagy
changed the title
|
Build SUCCESS |
|
This pull request fixes 1 alert when merging b48c6a6 into b1620af - view on LGTM.com fixed alerts:
Comment posted by LGTM.com |
furiel
approved these changes
Apr 3, 2019
alltilla
approved these changes
Apr 3, 2019
|
I have executed all additional test cases and they PASSED. |
gaborznagy
changed the title
gaborznagy
changed the title
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.
There is a race condition between scheduled I/O operations in worker threads and closing the idle connections (response/ack_timeout timer).
There is no protection - in afsocket source-driver - to not destroy connections(readers), which have scheduled I/O operation (not like when we reap writers on file dest. side).
The idle timer is not stopped, just when an I/O operation finished and we would like to re-schedule the next action on source side (update_watches() ).
The timer can trigger a connection closing, although there is schedule I/O operation.
Timer should be stopped when we schedule an I/O.
This way we eliminate the race condition between 2 sides.
We do stop I/O polling (see calling log_reader_stop_watches() in log_reader_io_handle_in()), so stopping the idle timer makes sens.
UPDATE:
About stop_idle_timer cals in update_watches:
Initially I thought that calling idle_timer is unnecessary in update_watches,
since we stop the timer in io_handle. I've ven put an assertion to the timer registration,
disabling timer re-registering.
I forgot that in case of suspend, we will call an update_watches again, where an idle_timer
registration will fail due to the assertion.