job-info: avoid error response on failed rpc #6502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: Some function parameter indentation is not aligned. Fix the tabbing.
garlick
approved these changes
Dec 12, 2024
| @@ -763,7 +768,7 @@ static void main_namespace_lookup_continuation (flux_future_t *f, void *arg) | |||
| "event", tok, toklen) < 0) { | |||
| flux_log_error (ctx->h, "%s: flux_respond_pack", | |||
| __FUNCTION__); | |||
| goto error; | |||
| goto cleanup; | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Good catch! Though maybe the skip_respond_error flag could be avoided if you just moved the error_cancel: code into the one block that uses it and goto cleanup from there?
src/modules/job-info/update.c
Outdated
Comment on lines
143
to
146
| json_t *context = NULL; | ||
| const char *errmsg = NULL; | ||
| const flux_msg_t *msg; | ||
| bool skip_respond_error = false; | ||
|
|
There was a problem hiding this comment.
Same comment on this one. Just call eventlog_watch_cancel() at each goto error_cancel site and goto error or goto cleanup from there as appropriate
Problem: Common usage in flux-core is to not respond with an RPC error (such as through flux_respond_error()) if a prior RPC attempt fails (such as through flux_respond_pack()). This pattern is violated in several places in job-info. Solution: If flux_respond_pack() fails, do not call flux_respond_error() in error handling paths. Fixes flux-framework#6498
chu11
force-pushed
the
issue6498_job_info_failed_rpc
branch
from
2354a68 to
15aad44
Compare
|
thanks! Re-pushed with those tweeks, will set MWP |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6502 +/- ##
==========================================
- Coverage 83.63% 83.61% -0.02%
==========================================
Files 525 525
Lines 87682 87682
==========================================
- Hits 73334 73317 -17
- Misses 14348 14365 +17
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem: Common usage in flux-core is to not respond with an RPC error
(such as through flux_respond_error()) if a prior RPC attempt fails (such
as through flux_respond_pack()). This pattern is violated in several
places in job-info.
Solution: If flux_respond_pack() fails, do not call flux_respond_error()
in error handling paths.
Fixes #6498