flux-core icon indicating copy to clipboard operation
flux-core copied to clipboard

libflux: add `flux_watcher_is_active(3)`

Open grondo opened this issue 1 year ago • 3 comments

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.

grondo avatar May 31 '24 19:05 grondo

I tried doing this a long time ago (although tried to do it differently) https://github.com/flux-framework/flux-core/pull/4173

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

chu11 avatar May 31 '24 19:05 chu11

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())

grondo avatar May 31 '24 19:05 grondo

For this to be safe in-tree, ev_flux, ev_zmq, ev_fbuf_read, ev_fbuf_write would also need to be special-cased, otherwise calling ev_is_active(w->data) would dereference an implementation specific structure assuming it is a struct ev_TYPE

Also, we exported API functions to allow out of tree watchers to be created. I shortsightedly did not pad struct flux_watcher_ops so adding a callback would break ABI.

Eh, not strictly related to this PR, but what if we clawed back the out of tree watcher support until it has stablized and there is a demonstrated need for it? Then we can have nice things like flux_watcher_is_active() without that complication?

garlick avatar Nov 13 '24 14:11 garlick

Sorry had meant to close this. It would be nice to have an issue open on this, though, since there are times when it would be very convenient to tell if a watcher has been started or not (as libev provides with ev_is_active())

grondo avatar Nov 13 '24 15:11 grondo

It would be nice to have an issue open on this, though, since there are times when it would be very convenient to tell if a watcher has been started or not (as libev provides with ev_is_active())

see #4156

garlick avatar Nov 13 '24 15:11 garlick

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.28%. Comparing base (e81e8f8) to head (b35d840). Report is 1383 commits behind head on master.

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 with missing lines Coverage Δ
src/common/libflux/reactor.c 96.19% <100.00%> (+0.06%) :arrow_up:

... and 11 files with indirect coverage changes

codecov[bot] avatar Feb 11 '25 15:02 codecov[bot]