Skip to content
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

Set the monitor_running event to unblock the main thread #633

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Jun 24, 2020

The do_before_work_loop starts the event monitor.

This spins up the thread and waits for it to be started. The amazon event catcher only sets the started event to true once an event is raised.

This means if you try to stop the event catcher before an event occurs you have to SIGKILL it as it will never shutdown cleanly.

@agrare agrare requested a review from Fryguy as a code owner June 24, 2020 15:30
@jrafanie
Copy link
Member

This fixes the issues I saw in ManageIQ/manageiq#20290 where we'd do event_catcher#kill on an amazon catcher and it would stick around for 10 minutes, the stopping timeout in pods. @carbonin suggested I try SIGTERM locally and indeed SIGTERM was seemingly being ignored on my amazon event catcher.

@agrare tracked this down to where we trap interrupts and later react to them once do_work completes. Since do_work calls start_event_monitor in the base event catcher and start_event_monitor won't start until the first event is received, it never gets the exit in the event of a tiny setup with very few events... it couldn't be more obvious now...

@djberg96
Copy link
Contributor

@agrare Don't suppose there's a way to spec this, is there?

@agrare agrare added the bug label Jun 24, 2020
@agrare
Copy link
Member Author

agrare commented Jun 24, 2020

@djberg96 the existing tests for Worker Runners are pretty hacky and have to stub a bunch of stuff to not actually start up a worker, so there isn't an easy way to spec this

Without setting the monitor_running concurrent event the main
event_catcher worker thread doesn't ever leave the do_before_work_loop
which means it can't be shutdown cleanly until the first event is
raised.
@agrare agrare force-pushed the event_monitor_never_sets_monitor_running branch from 475b1d1 to d8308e7 Compare June 25, 2020 14:39
@agrare
Copy link
Member Author

agrare commented Jun 25, 2020

@miq-bot
Copy link
Member

miq-bot commented Jun 25, 2020

Checked commit agrare@d8308e7 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 👍

@djberg96
Copy link
Contributor

@agrare Could there be a connection between this and #620 ?

@agrare
Copy link
Member Author

agrare commented Jun 25, 2020

Hm that's a good question, my first reaction was no because once an event is caught and yielded to that block it would set the event that the monitor thread was started. There could be some weirdness though in the fact that the main runner thread isn't in the do_work loop where it actually processes events until that event is set.

@simaishi
Copy link
Contributor

simaishi commented Jul 1, 2020

Jansa backport details:

$ git log -1
commit dd5052771d9ab30b215be05dbc92dac6c3ef60fa
Author: Oleg Barenboim <chessbyte@gmail.com>
Date:   Thu Jun 25 12:09:17 2020 -0400

    Merge pull request #633 from agrare/event_monitor_never_sets_monitor_running

    Set the monitor_running event to unblock the main thread

    (cherry picked from commit 3d9f0905fbb5561ccf452be25496759385a71fd8)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants