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: add flux_watcher_is_active(3) #6024

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

grondo
Copy link
Contributor

@grondo grondo commented May 31, 2024

While working on something I noticed that there was no libflux version of the ev_is_active() call, which could come in handy to check if a watcher has been started.

This simple PR just adds flux_watcher_is_active(3), tests, and documentation.

Problem: There is no way to test if a flux_watcher_t is stopped
or started.

Add flux_watcher_is_active(3), which wraps ev_is_active(3).
Problem: There are no unit tests for flux_watcher_is_active(3).

Ensure flux_watcher_is_active() returns false before a watcher is
started, and true afterwards.

Add a todo for periodic watchers, which do not seem to follow this
rule.
Problem: flux_watcher_is_active(3) is not documented.

Add it to flux_watcher_start.rst.
@chu11
Copy link
Member

chu11 commented May 31, 2024

I tried doing this a long time ago (although tried to do it differently) #4173

I think you handled the gotchas, but just in case you wanted to review comments there

Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.28%. Comparing base (e81e8f8) to head (b35d840).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6024      +/-   ##
==========================================
- Coverage   83.29%   83.28%   -0.01%     
==========================================
  Files         518      518              
  Lines       83514    83520       +6     
==========================================
+ Hits        69560    69563       +3     
- Misses      13954    13957       +3     
Files Coverage Δ
src/common/libflux/reactor.c 96.19% <100.00%> (+0.06%) ⬆️

... and 11 files with indirect coverage changes

@grondo
Copy link
Contributor Author

grondo commented May 31, 2024

I had forgotten that sorry!

The use case that came up is that I was going to add per-job data to flux module stats job-exec and wanted to test if the kill_timer or the kill_shell_timer watchers were active rather than using a specific flag or other method.

However, if this is still deemed more work than its worth, then I can close this one for now (I don't think it is worth it if we have to add a now method to flux_watcher_t that calls ev_is_active())

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

Successfully merging this pull request may close these issues.

2 participants