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

Issue 495 - re-architect behavior of numerous rpc/msg functions #1009

Merged
merged 9 commits into from Mar 21, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Mar 17, 2017

Re-architect the API style of flux_response_decode(), flux_rpc_get(), flux_request_decode(), and flux_event_decode(). Previously, user was required to pass in a payload pointer that matched the message payload existence (i.e. non-NULL pointer if payload is in the message, NULL pointer if a payload is not available). Mismatched pointer + payload existence lead to EPROTO errors.

Now, the user can safely pass in a pointer or not, regardless if a payload is available. For example, if a payload is sent, but the caller does not care about it, the caller can now safely pass in a NULL pointer indicating they do not care about the payload. If the user was expecting a payload, they should check the payload pointer for != NULL to ensure payload data was received.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 17, 2017

Coverage Status

Coverage decreased (-0.05%) to 77.56% when pulling 1ee7b03 on chu11:issue495 into 2d3820a on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 17, 2017

Codecov Report

Merging #1009 into master will decrease coverage by 0.31%.
The diff coverage is 68.42%.

@@            Coverage Diff             @@
##           master    #1009      +/-   ##
==========================================
- Coverage   77.64%   77.33%   -0.32%     
==========================================
  Files         151      151              
  Lines       25751    25776      +25     
==========================================
- Hits        19995    19933      -62     
- Misses       5756     5843      +87
Impacted Files Coverage Δ
src/common/libflux/response.c 83.76% <ø> (-1.24%) ⬇️
src/common/libflux/event.c 91.22% <ø> (-0.44%) ⬇️
src/common/libflux/request.c 89.04% <ø> (-0.44%) ⬇️
src/common/libflux/module.c 48.63% <0%> (-30.63%) ⬇️
src/common/libflux/reparent.c 0% <0%> (-42.86%) ⬇️
src/connectors/ssh/ssh.c 85.23% <100%> (ø) ⬆️
src/modules/kvs/kvs.c 80.62% <100%> (+0.02%) ⬆️
src/modules/aggregator/aggregator.c 77.17% <100%> (ø) ⬆️
src/broker/ping.c 66.03% <100%> (ø) ⬆️
src/modules/wreck/job.c 71.37% <100%> (ø) ⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e07ed32...67e8789. Read the comment docs.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 17, 2017

Ah nice to finally get this done.

This puzzled me a bit though

ok ((r = flux_rpcf (h, "rpcftest.hello", FLUX_NODEID_ANY, 0, "{}")) != NULL,
        "flux_rpcf with no payload when none is expected works");

An empty json object { } would still generate a payload (as in a message that flux_msg_has_payload() would return true on), wouldn't it?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 17, 2017

@garlick Ahh, you're right. What I really meant was a payload with the minimal JSON, which is an empty object. I'll adjust the text.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 17, 2017

Looking through code coverage, mostly appears to be fine. Error branches being missed as normal, but did miss a block chunk in libflux/module.c and libflux/reparent.c b/c some functions weren't covered. I'll see if I can improve that.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 17, 2017

I see other places where a format string of "{ }" is used in what appears to be an empty payload test. I'll wait for your next update and then go through it and comment on the individual diffs.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 17, 2017

@garlick that one particular test maybe isn't necessary. It was sort of a "buildup" to the tests in 65c993f. We could maybe just axe it.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 17, 2017

OK by me!

@chu11 chu11 force-pushed the chu11:issue495 branch from 1ee7b03 to ae51fb8 Mar 18, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 18, 2017

Just pushed PR #1011, which will hopefully fix up some of the coverage issues once i rebase.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 18, 2017

Coverage Status

Coverage increased (+0.03%) to 77.647% when pulling ae51fb8 on chu11:issue495 into 2d3820a on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 21, 2017

Just had a spin through this and it looks good.

chu11 added some commits Mar 1, 2017

test/rpc: Add remote EPROTO tests
Add remote EPROTO tests for flux_rpcf and flux_rpcf_multi
test/rpc: Add local EPROTO tests
Add tests to check for EPROTO locally, when a user incorrectly
assumes there is no payload (when there is one) or when a user
incorrectly assumes a payload (when there isn't one).
libflux/response: Rearch flux_response_decode
Rearchitect flux_response_decode.  User can now pass in a NULL
or non-NULL json_str pointer.  The function will succeed regardless
if a payload is available or not.  The function will no longer return
EPROTO under a payload & pointer mismatch.  User is responsible to
check for NULL or non-NULL per their expectations.
Update usage of flux_response_decode()
Per recent function behavior change, update flux_response_decode()
usage appropriately.  Most commonly, check that the payload
pointer is non-NULL before continuing.  Or if a payload
is not used, do not pass in a pointer.
Update usage of flux_rpc_get()
Per recent function behavior change to flux_response_decode(),
adjust usage of flux_rpc_get() appropriately.  Most commonly,
check that the payload pointer is non-NULL before continuing.
Or if a payload is not used, do not pass in a pointer.
libflux/request: Rearch flux_request_decode
Rearchitect flux_request_decode.  User can now pass in a NULL
or non-NULL json_str pointer.  The function will succeed regardless
if a payload is available or not.  The function will no longer return
EPROTO under a payload & pointer mismatch.  User is responsible to
check for NULL or non-NULL per their expectations.

Fixes #495
Update usage of flux_request_decode()
Per recent function behavior change, update flux_request_decode()
usage appropriately.  Most commonly, check that the payload
pointer is non-NULL before continuing.  Or if a payload
is not used, do not pass in a pointer.
libflux/event: Rearch flux_event_decode
Rearchitect flux_event_decode.  User can now pass in a NULL
or non-NULL json_str pointer.  The function will succeed regardless
if a payload is available or not.  The function will no longer return
EPROTO under a payload & pointer mismatch.  User is responsible to
check for NULL or non-NULL per their expectations.
modules/kvs: Update usage of flux_event_decode()
Per recent function behavior change, update flux_event_decode()
usage appropriately.  In particular, check that the payload
pointer is non-NULL before continuing.

@chu11 chu11 force-pushed the chu11:issue495 branch from ae51fb8 to 67e8789 Mar 21, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 21, 2017

Coverage Status

Coverage decreased (-0.06%) to 77.896% when pulling 67e8789 on chu11:issue495 into e07ed32 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 21, 2017

Woo-hoo! Enough coverage now.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 21, 2017

Great work @chu11! Merging!

@garlick garlick merged commit a549739 into flux-framework:master Mar 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.06%) to 77.896%
Details

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.