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

libflux/reactor: add flux_reactor_active_incref(), _decref() #2387

Merged
merged 6 commits into from Sep 21, 2019

Conversation

@garlick
Copy link
Member

commented Sep 21, 2019

Per discussion in #2289, add two new reactor functions to manipulate active watcher counts. When you want to start a watcher that should not increase the active watcher count, use:

flux_watcher_start()
flux_reactor_active_decref()

to stop it use:

flux_reactor_active_incref()
flux_watcher_stop()

This allowed for a bit of cleanup in the flux job attach command, which needed to externally track the "real" active watchers in order to stop some signal watchers that fall into the above category.

There's a loose PMI fix in here too (very minor, but was bugging me).

garlick added 6 commits Sep 21, 2019
Problem: it can be cumbersome to organize reactor exit
when some watchers (like signal watchers or some message
handlers) do not become naturally inactive.

Add flux_reactor_active_incref() and flux_reactor_active_decref()
which internally call ev_ref() and ev_unref().

Fixes #2289
Remove the logic in then attach subcommand for stopping
signal watchers when it's time to exit the reactor loop.
Instead, simply ensure those watchers do not affect the
active watcher count with the idiom:

flux_watcher_start()
flux_reactor_active_decref().

flux_reactor_active_incref()
flux_watcher_stop()
Add descriptions and stubs for flux_reactor_active_incref()
and flux_reactor_active_decref().
Problem: make clean doesn't remove flux_future_fulfill_with.3.

Add stub to MAN3_FILES_SECONDARY and add production rule.
Problem: PMI2_Info_GetJobAttr() did not set 'found' parameter
to zero in one unlikely error path.

Ensure 'found' is set in this case too.
@codecov-io

This comment has been minimized.

Copy link

commented Sep 21, 2019

Codecov Report

Merging #2387 into master will decrease coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #2387      +/-   ##
==========================================
- Coverage   80.92%   80.91%   -0.02%     
==========================================
  Files         222      222              
  Lines       34991    34986       -5     
==========================================
- Hits        28317    28308       -9     
- Misses       6674     6678       +4
Impacted Files Coverage Δ
src/common/libpmi/pmi2.c 83.92% <ø> (+0.74%) ⬆️
src/common/libflux/reactor.c 92.18% <100%> (+0.1%) ⬆️
src/cmd/flux-job.c 84.57% <66.66%> (-0.66%) ⬇️
src/common/libsubprocess/local.c 79.79% <0%> (-0.35%) ⬇️
src/broker/module.c 74.94% <0%> (-0.24%) ⬇️
src/cmd/flux-module.c 84.19% <0%> (ø) ⬆️
src/common/libflux/message.c 80.5% <0%> (ø) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

Awesome!

Just a thought (and it may be naive, I apologize if so), but would it be easier if this were implemented as a flag on the watcher and the ev_ref()/unref were done under the covers automatically during start/stop?

@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2019

I thought about that, but I was a bit worried how to handle e.g. timer watchers that can stop themselves in libev land. It seemed better to make the refcount tweaking explicit like libev did...

@grondo

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

Ok, that's a good point. Do you have the same problem with timers even with the explicit tweaking?

My only worry is that incref/stop phase of this approach will be pretty easily missed.

However, it is much better than the status quo.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2019

Yes (one-shot) timers are tricky either way, but at least the foot gun is out in the open this way :-)

@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2019

BTW this won't work for message watchers, because they are tied to an internal "dispatcher" object that holds one reference on the reactor (through the single "handle watcher"). There would need to be a separate thing done for message watchers if/when that case arises.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

Ok, literally exposing the underlying libev call seems most reasonable. Thanks!

@grondo grondo merged commit 2f03500 into flux-framework:master Sep 21, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 88.88% of diff hit (target 80.92%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +7.96% compared to 1e9f645
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2019

Thanks!

@garlick garlick deleted the garlick:active_incref branch Sep 22, 2019
@garlick garlick referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.