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

job-info: Watcher - cancel lookup on several error paths #2303

Merged
merged 4 commits into from Aug 13, 2019

Conversation

@chu11
Copy link
Contributor

commented Aug 12, 2019

I realized in the job-info eventlog watch code, I don't cancel a lookup under several circumstances, such as when the user has a permission issue. So this could lead to a matchtag leak. So added a lookup cancellation call in the error path of some cleanup.

Also includes some random cleanup fixes.

@chu11 chu11 force-pushed the chu11:job_info_watcher_cancel_lookup branch 2 times, most recently from 2066c78 to d002973 Aug 12, 2019
chu11 added 3 commits Aug 12, 2019
Problem: Under several circumstances, such as when there is a permission
issue, the job-info module will respond to the original caller with an error,
and then destroy the flux future watching the KVS key.  This could lead to a
matchtag leak, b/c the lookup is never actually canceled.

Solution: In these particular circumstances, send a lookup cancellation before
destroying the future.
There is no need to send error responses to caller directly on ENODATA or
watch cancellation.  Instead just fallthrough to error path via 'goto error',
which will send error based on errno value.
@chu11 chu11 force-pushed the chu11:job_info_watcher_cancel_lookup branch from d002973 to f6d6d9b Aug 13, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

found 1 more dead code in lookup.c, squashed it into another cleanup in this PR

@codecov-io

This comment has been minimized.

Copy link

commented Aug 13, 2019

Codecov Report

Merging #2303 into master will decrease coverage by <.01%.
The diff coverage is 60%.

@@            Coverage Diff            @@
##           master   #2303      +/-   ##
=========================================
- Coverage    80.9%   80.9%   -0.01%     
=========================================
  Files         214     214              
  Lines       33681   33677       -4     
=========================================
- Hits        27249   27245       -4     
  Misses       6432    6432
Impacted Files Coverage Δ
src/modules/job-info/lookup.c 65.87% <ø> (+0.75%) ⬆️
src/modules/job-info/watch.c 69.23% <60%> (+1.43%) ⬆️
src/shell/shell.c 80.51% <0%> (-0.52%) ⬇️
src/modules/kvs-watch/kvs-watch.c 79.2% <0%> (-0.42%) ⬇️
src/common/libsubprocess/local.c 79.79% <0%> (-0.35%) ⬇️
src/common/libflux/message.c 80.5% <0%> (-0.26%) ⬇️
src/cmd/flux-job.c 85.64% <0%> (+0.31%) ⬆️
Copy link
Member

left a comment

LGTM. Feel free to set merge-when-passing label when you're done fine tuning.

@mergify mergify bot merged commit 624aa04 into flux-framework:master Aug 13, 2019
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 60% of diff hit (target 80.9%)
Details
Summary 1 rule matches
Details
codecov/project 80.9% (-0.01%) compared to 405bac8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@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.