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/rpc: add FLUX_RPC_STREAMING and FLUX_O_MATCHDEBUG #1942

Merged
merged 12 commits into from Jan 23, 2019

Conversation

@garlick
Copy link
Member

commented Jan 21, 2019

This PR adds a FLUX_RPC_STREAMING flag that indicates that multiple responses are expected for this RPC. In the RPC destructor, if the future was not fulfilled with an end-of-stream (any error response, as described in RFC 6), the matchtag is leaked to prevent it from being recycled with old responses still arriving.

In addition, some stderr debugging is added to rpc and mrpc for leaky matchtags if the flux_t handle has the FLUX_O_MATCHDEBUG flag set. (the FLUX_HANDLE_MATCHDEBUG environment variable, if set, causes this flag to be set automatically, similar to how FLUX_HANDLE_TRACE works)

flux_kvs_lookup() now sets FLUX_RPC_STREAMING if the FLUX_KVS_WATCH flag was used, so that interface should become a bit safer. That's the only place where we are using the RFC 6 end-of-stream convention that I'm aware of - other places like subprocess use different termination schemes.

Maybe addresses #1937 and #1938?

@codecov-io

This comment has been minimized.

Copy link

commented Jan 21, 2019

Codecov Report

Merging #1942 into master will increase coverage by <.01%.
The diff coverage is 97.5%.

@@            Coverage Diff             @@
##           master    #1942      +/-   ##
==========================================
+ Coverage   80.07%   80.07%   +<.01%     
==========================================
  Files         195      195              
  Lines       35078    35110      +32     
==========================================
+ Hits        28088    28114      +26     
- Misses       6990     6996       +6
Impacted Files Coverage Δ
src/common/libkvs/kvs_getroot.c 86.66% <100%> (-0.22%) ⬇️
src/common/libflux/rpc.c 92.99% <100%> (+0.74%) ⬆️
src/common/libflux/mrpc.c 87.74% <100%> (+0.19%) ⬆️
src/common/libkvs/kvs_lookup.c 80.76% <100%> (+0.32%) ⬆️
src/common/libflux/handle.c 84.16% <90.9%> (+0.17%) ⬆️
src/common/libflux/response.c 81.48% <0%> (-1.24%) ⬇️
src/modules/kvs-watch/kvs-watch.c 78.88% <0%> (-0.89%) ⬇️
src/common/libflux/message.c 81.27% <0%> (-0.25%) ⬇️
src/common/libzio/zio.c 74% <0%> (-0.23%) ⬇️
src/cmd/flux-event.c 77.97% <0%> (ø) ⬆️
... and 2 more
@garlick garlick force-pushed the garlick:matchtag_debug branch from 2a9ddb4 to 2c3c907 Jan 22, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

Fixed a typo introduced in the leak comment in rpc.c, squashed, rebased.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

Just realized I didn't add logging at flux_close() time for unfreed matchtags as proposed in #1938, so let me see if I can add that.

garlick added 12 commits Jan 20, 2019
If a matchtag is leaked, and the user set the FLUX_O_MATCHDEBUG
flag, log the event to stderr.
Problem: If an MRPC is destroyed before all responses are received,
the matchtag is leaked to prevent it from being reused in a new
request.  This may be normal or due to a programming error.

If flux_t handle has the FLUX_O_MATCHDEBUG flag, log this to stderr.
Problem: although logic exists to leak a matchtag if an RPC
future is destroyed without being fulfilled, this is insufficient
for streaming RPCs, which may still have responses in flight
even when the future is fulfilled.

Add a FLUX_RPC_STREAMING flag to designate an RPC as
expecting multiple responses, then leak the matchtag if it
has not been fulfilled with an error, which signifies
end-of-stream per RFC 6.
Add the FLUX_RPC_STREAMING flag to the KVS lookup RPC,
if the FLUX_KVS_WATCH flag is set.

This will enable matchtag reuse safety and FLUX_O_DEBUG
for code that improperly destroys the lookup future with
lookup responses potentially in flight.
Problem: passing topic string to rpc function in a variable
is a vetige of previous code that allowed get root "watching".

Pass the topic string directly into the RPC function as we
do most other places.
If the tagpool has allocated matchtags when it is destroyed,
and the handle has the FLUX_O_MATCHDEBUG flag, log this to
stderr.
@garlick garlick force-pushed the garlick:matchtag_debug branch from 51ce946 to 0693888 Jan 22, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

Rebased on current master.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

I think this could go in. It's pretty low risk.

@grondo grondo merged commit 9822c63 into flux-framework:master Jan 23, 2019
3 checks passed
3 checks passed
codecov/patch 97.5% of diff hit (target 80.07%)
Details
codecov/project 80.07% (+<.01%) compared to 546f945
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

Thanks!

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.