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: reclaim leaked matchtags from late arriving responses #2153

Merged
merged 8 commits into from May 13, 2019

Conversation

@garlick
Copy link
Member

commented May 9, 2019

If an RPC future is destroyed before it has received all its responses, the matchtag is leaked. This PR reclaims the matchtag if future is destroyed:

  1. before the RPC is sent (setup failure)
  2. after streaming response is received, but is not the "top" result in the queue of fulfillments
  3. before response is received to a normal RPC, but the response subsequently arrives
  4. before error response is received to a streaming RPC, but the error response subsequently arrives

This code cannot reclaim matchtags from abandoned mrpcs (but they are few), nor will it reclaim matchtags from late arriving responses after the dispatcher is destroyed, which will happen when the last message handler is unregistered. (I'm trying to figure out how to close that last gap)

To make this work, I had to add a new message flag to distinguish streaming responses from non-streaming responses in the absence of any other state since the future's message handler was destroyed earlier. I think this might be useful later on if we need to hold state in the broker for communicating endpoints for recovery purposes.

While I was mucking around with message flags, I also cleaned up the accessors for nodeid in messages, which included limited flags access which seems a bit silly.

In a future PR, if I can get some issues sorted out with the design, the destructor for an RPC will issue a generic cancellation request if it is destroyed with outstanding requests, and then this code would mop up the leaked matchtag once the final response is received. At least that is my plan.

This still needs some tests, but feedback welcome!

@garlick

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Add unit test coverage for the dispatch code for recovering lost matchtags.

nor will it reclaim matchtags from late arriving responses after the dispatcher is destroyed, which will happen when the last message handler is unregistered. (I'm trying to figure out how to close that last gap)

This was not strictly true. If another message handler is registered after the dispatcher is destroyed, the dispatcher is recreated and lost matchtags from before are handled properly.

(The dispatcher is destroyed on last use so that it can unregister its "handle watcher" and allow the reactor to terminate naturally when the last watcher is stopped/destroyed.)

@garlick garlick force-pushed the garlick:matchtag_reclaim branch from 29ad811 to f57f2b5 May 9, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

With the changes proposed thus far, there is an increased likelihood that someone might encounter an improperly recycled matchtag due to the increased aggressiveness reclaiming them.

To counter that, I've added checks to the streaming RPC services in kvs-watch and job-info that fail a request with EPROTO if it doesn't include the FLUX_MSGFLAG_STREAMING flag in the request. An immediate error return ensures that if the response is "orphaned", the matchtag reclaim logic will do the right thing even without the STREAMING flag set in the response.

To make checking the flag a bit easier I added the flux_msg_is_streaming() accessor, and added some missing unit tests (and parameter validation) for message flag manipulation functions.

In our discussion face to face earlier today, it was suggested that we might want to add a flag that would allow message handlers to be registered with built-in error handling for missing FLUX_MSGFLAG_STREAMING. It's a good idea, but I'm not sure it's going to make the problem that much easier, and as there were only two live cases of services needing this and one required a conditional to be tested to determine whether it was streaming or not, I elected to go with the simpler approach for now. (this does not apply to the "streaming" responses in the job manager interfaces, as those do not use matchtags)

I added a bit of manpage commentary also.

@garlick

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Just noticed a typo in a commit message for f57f2b5 so let me fix that...

@garlick garlick force-pushed the garlick:matchtag_reclaim branch from 6552000 to bb170b3 May 10, 2019
@@ -37,6 +37,7 @@ struct flux_rpc {
uint32_t matchtag;
int flags;
flux_future_t *f;
int sent:1;

This comment has been minimized.

Copy link
@SteVwonder

SteVwonder May 10, 2019

Member

I was completely unaware of the : bit field syntax in a C struct. TIL! Neat!

Any reason to use :1 over bool? From the sparse info I can find online, it looks like bit fields have a higher runtime cost than direct member access.

This comment has been minimized.

Copy link
@garlick

garlick May 10, 2019

Author Member

Bitfields for boolean values may pack better in some cases, e.g. see 'struct job' here for one instance where I figured it might reduce memory footprint usefully.

In this case you cited, it's definitely not helping. I'll switch to bool.

@garlick

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Hmm got a valgrind timeout in travis:

==32605== For counts of detected and suppressed errors, rerun with: -v
==32605== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
flux-start: 0 (pid 32604) Killed
not ok 1 - valgrind reports no new errors on 2 broker run

I'm guessing that's got nothing to do with this PR...

@garlick garlick referenced this pull request May 10, 2019
@garlick garlick force-pushed the garlick:matchtag_reclaim branch from bb170b3 to 8f01d4d May 10, 2019
@grondo

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Hmm got a valgrind timeout in travis:

I've been seeing this every once in awhile. We might need to increase the shutdown grace time even further?

garlick added 8 commits May 8, 2019
Problem: flux_msg_get_nodeid() and flux_msg_set_nodeid()
accept both nodeid and flags, but there are now separate
accessors for flags.

The flags were added to the nodeid accessors to support
FLUX_MSGFLAG_UPSTREAM before the general flags accessors
were added.  Drop the flags argument from the nodeid
accessors.

Update users and tests to make use of the flags accessors
where they were formerly accessed via the nodeid accessor.
Problem: when looking at an orphaned response message,
one cannot decide whether it is safe to retire the
matcthag without knowing whether it is a "regular"
RPC (any response terminates) or "streaming" RPC
(error response terminates).

Set FLUX_MSGFLAG_STREAMING in the request/response
message if it is from a streaming RPC.

Add convenient accessors for FLUX_MSGFLAG_STREAMING:
flux_msg_is_streaming()
flux_msg_set_streaming()

Add check for bad params in
flux_msg_get_flags()
flux_msg_set_flags().

Add unit test coverage.
Problem: we need a way to test if a matchtag belongs
to an mrpc or a regular rpc to decide whether an orphaned
one can be retired, but there is no way to ask what type
it is because details are hidden in the tagpool "class".

Add tagpool_group(), wrapped with flux_matchtag_group().

Add unit tests.
Problem: if an RPC future is destroyed before its
last response is received, the RPC code leaks the matchtag.
If the response is later received, no attempt is made
to reclaim the matchtag.

Look at unclaimed response messages before dropping them
and see if it contains a live matchtag.  If we have enough
info in the response to determine if it is the last one,
free the matchtag.
Problem: There are two (unlikely) cases not handled in
the current code where a matchtag is leaked unnecessarily.
1) If rpc is destroyed before request is sent due to
failure between matchtag allocation and flux_send();
2) If the future fulfillment backlog contains a terminating
response that has not yet been consumed.

Handle these cases.
Add unit test coverage for the dispatcher reclaiming lost
matchtags from orphaned responses from both normal and
streaming RPCs.  In additon:
- ensure that an orphaned response to a streaming RPC
  does not trigger reclaim unless it is an error response
- ensure that an oprphaned response to an mrpc
  does not trigger reclaim, since that is currently
  impossible to do safely.
Problem: if a request is sent to a "streaming" RPC
service without the FLUX_MSGFLAG_STREAMING flag,
an orphaned, non-error response might prematurely trigger
matchtag reclaimation.

Services that send multiple responses should check for
the flag early (before sending non-error responses),
and respond with an EPROTO error response if the check fails.

An error response terminates both streaming and non-streaming
RPCs, so the fact that the STREAMING flag is not set in the
EPROTO response causes no confusion.

Add this check to the following services:
- rpc unit test (rpctest.multi)
- job-info.eventlog-watch
- kvs-watch.lookup (with FLUX_KVS_WATCH only)
Add more explanation of how streaming RPCs should be
handled, including a section describing how matchtags
are reclaimed when RPCs are destroyed prematurely
in flux_rpc(3).
@garlick garlick force-pushed the garlick:matchtag_reclaim branch from 8f01d4d to d813b5a May 11, 2019
@codecov-io

This comment has been minimized.

Copy link

commented May 11, 2019

Codecov Report

Merging #2153 into master will increase coverage by 0.03%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #2153      +/-   ##
==========================================
+ Coverage   80.43%   80.47%   +0.03%     
==========================================
  Files         200      200              
  Lines       31763    31817      +54     
==========================================
+ Hits        25549    25605      +56     
+ Misses       6214     6212       -2
Impacted Files Coverage Δ
src/modules/kvs-watch/kvs-watch.c 79.62% <100%> (+0.21%) ⬆️
src/common/libflux/msg_handler.c 89.96% <100%> (-0.28%) ⬇️
src/common/libflux/handle.c 85.81% <100%> (+2.1%) ⬆️
src/common/libflux/mrpc.c 88.97% <100%> (+0.04%) ⬆️
src/broker/broker.c 78.08% <100%> (+0.02%) ⬆️
src/modules/connector-local/local.c 73.82% <100%> (+0.19%) ⬆️
src/common/libflux/tagpool.c 94.38% <100%> (+0.19%) ⬆️
src/common/libflux/rpc.c 93.64% <100%> (+0.26%) ⬆️
src/bindings/lua/flux-lua.c 87.17% <33.33%> (-0.18%) ⬇️
src/modules/job-info/watch.c 67.79% <50%> (-1.23%) ⬇️
... and 5 more
@grondo

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Is this ready for a merge?

@garlick

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

I think so, yes.

@grondo grondo merged commit 4a0dc35 into flux-framework:master May 13, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 90.9% of diff hit (target 80.43%)
Details
codecov/project 80.47% (+0.03%) compared to 34b8abe
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick garlick deleted the garlick:matchtag_reclaim branch May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.