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

make flux_msg_t a bonafide type, add jansson payload accessors #857

Merged
merged 29 commits into from Oct 20, 2016

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Oct 18, 2016

Posted early to get any feedback. There are still tests to write and docs to update.

flux_msg_t was an alias for zmsg_t during transition to a "real" native flux message type. This PR finally converts remaining zmsg_t users to native flux message functions. Only the minimum work was done to achieve this goal, i.e. the old czmq "destroy on send" idiom is still used in the broker and the lua bindings; however that can be cleaned up in another PR.

Freeing flux_msg_t to define its own opaque type allowed it to internally grow a json_t member for caching decoded JSON state, which in turn allowed accessors in the style of jansson pack/unpack to be implemented, where returned data is "owned" by the flux_msg_t.

Additional wrappers were added in the request, response, and event encoding/decoding functions that leverage this capability. The new flux_rpcf() and flux_rpc_getf() functions were simplified to use these rather than their own json_t member in flux_rpc_t.

Here are the new interfaces. (Honestly I may have gone slightly overboard adding interfaces that are not likely to be used much!)

message.h

int flux_msg_set_payload_vjsonf (flux_msg_t *msg, const char *fmt, va_list ap);
int flux_msg_set_payload_jsonf (flux_msg_t *msg, const char *fmt, ...);
int flux_msg_get_payload_vjsonf (flux_msg_t *msg, const char *fmt, va_list ap);
int flux_msg_get_payload_jsonf (flux_msg_t *msg, const char *fmt, ...);

request.h

int flux_request_vdecodef (flux_msg_t *msg, const char **topic,
                           const char *fmt, va_list ap);
int flux_request_decodef (flux_msg_t *msg, const char **topic,
                          const char *fmt, ...);
flux_msg_t *flux_request_vencodef (const char *topic,
                                   const char *fmt, va_list ap);
flux_msg_t *flux_request_encodef (const char *topic, const char *fmt, ...);

response.h

int flux_response_vdecodef (flux_msg_t *msg, const char **topic,
                            const char *fmt, va_list ap);
int flux_response_decodef (flux_msg_t *msg, const char **topic,
                           const char *fmt, ...);
flux_msg_t *flux_response_vencodef (const char *topic,
                                    const char *fmt, va_list ap);
flux_msg_t *flux_response_encodef (const char *topic, const char *fmt, ...);
int flux_vrespondf (flux_t *h, const flux_msg_t *request,
                    const char *fmt, va_list ap);
int flux_respondf (flux_t *h, const flux_msg_t *request,
                   const char *fmt, ...);

event.h

int flux_event_vdecodef (flux_msg_t *msg, const char **topic,
                         const char *fmt, va_list ap);
int flux_event_decodef (flux_msg_t *msg, const char **topic,
                         const char *fmt, ...);
flux_msg_t *flux_event_vencodef (const char *topic, const char *fmt, va_list ap);
flux_msg_t *flux_event_encodef (const char *topic, const char *fmt, ...);

@garlick garlick added the review label Oct 18, 2016

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 18, 2016

Wow, nice!

Dumb question -- all the decode/decodef functions have the same prototype.. would there be a way to generically decode a message via one function pair, e.g. flux_msg_decodef()/vdecodef(), or is it considered unsafe?

You could think of it similar to the way flux_watcher_t currently works, which nicely keeps the number of member functions low...

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 18, 2016

You make a good point @grondo. In fact, the main difference between the decoding functions across message types, is that they fail if the message isn't the expected type. But given that we for the most part dispatch/match messages based on having already checked that, it's a bit silly. For the same reason we could lose the topic argument.

We could maybe get rid of all of them and have people use the existing type-agnostic accessors?

int flux_msg_get_payload_json (const flux_msg_t *msg, const char **json_str);
int flux_msg_get_payload_vjsonf (flux_msg_t *msg, const char *fmt, va_list ap);
int flux_msg_get_payload_jsonf (flux_msg_t *msg, const char *fmt, ...);
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 18, 2016

Oh, yeah, duh that makes sense, though those accessor functions have very long names, but I can't think of a good way to shorten them.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 18, 2016

Maybe drop the _payload part?

Let me play around and see what I can come up with.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 18, 2016

Yeah, in any event this is turning out pretty nice.

@garlick garlick force-pushed the garlick:flux_msg_t branch from bba7911 to 3d21bbc Oct 19, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 19, 2016

Current coverage is 71.82% (diff: 81.60%)

Merging #857 into master will increase coverage by 0.09%

@@             master       #857   diff @@
==========================================
  Files           156        156          
  Lines         26803      26932   +129   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19226      19344   +118   
- Misses         7577       7588    +11   
  Partials          0          0          
Diff Coverage File Path
0% src/common/libflux/security.c
••• 33% src/broker/overlay.c
••••• 50% src/modules/wreck/wrexecd.c
•••••• 60% src/common/libcompat/request.c
••••••• 73% src/common/libflux/message.c
•••••••• 85% src/bindings/lua/flux-lua.c
•••••••• 86% src/common/libflux/response.c
•••••••• 89% src/broker/broker.c
••••••••• 92% src/common/libflux/request.c
••••••••• 97% src/common/libflux/event.c

Review all 19 files changed

Powered by Codecov. Last update 0eb8cdc...61ffe2c

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 19, 2016

Coverage Status

Changes Unknown when pulling 3d21bbc on garlick:flux_msg_t into * on flux-framework:master*.

@garlick garlick force-pushed the garlick:flux_msg_t branch 2 times, most recently from d874c5d to dcfdac8 Oct 19, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 19, 2016

Coverage Status

Changes Unknown when pulling d874c5d on garlick:flux_msg_t into * on flux-framework:master*.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 19, 2016

Coverage Status

Changes Unknown when pulling 856d265 on garlick:flux_msg_t into * on flux-framework:master*.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 19, 2016

This might be ready for another look. I pared down the API changes to:

message.h

int flux_msg_set_json (flux_msg_t *msg, const char *json_str); // shortened name
int flux_msg_set_vjsonf (flux_msg_t *msg, const char *fmt, va_list ap);
int flux_msg_set_jsonf (flux_msg_t *msg, const char *fmt, ...);

int flux_msg_get_json (const flux_msg_t *msg, const char **json_str); // shortened name
int flux_msg_get_vjsonf (const flux_msg_t *msg, const char *fmt, va_list ap);
int flux_msg_get_jsonf (const flux_msg_t *msg, const char *fmt, ...);

event.h

int flux_event_decodef (const flux_msg_t *msg, const char **topic,
                        const char *fmt, ...);
flux_msg_t *flux_event_encodef (const char *topic, const char *fmt, ...);

request.h

int flux_request_decodef (const flux_msg_t *msg, const char **topic,
                          const char *fmt, ...);

response.h

int flux_respondf (flux_t *h, const flux_msg_t *request,
                   const char *fmt, ...);

and added test coverage.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 19, 2016

Coverage Status

Changes Unknown when pulling 856d265 on garlick:flux_msg_t into * on flux-framework:master*.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 19, 2016

Nice!! One totally captious comment that I even hesitate to mention -- but since get_json and set_json can be considered the method names for the flux_msg class, would it make more sense for the varargs versions to be flux_msg_vget_json() and flux_msg_vset_json()? I find that easier to mentally grasp than get/set_vjson.

Thanks and I'll take my response off the air.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 19, 2016

OK :-) I struggled with that one. That sounds good.

@garlick garlick force-pushed the garlick:flux_msg_t branch from 856d265 to f9dd62e Oct 19, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 19, 2016

Rebased and varargs functions changed as suggested by @grondo.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 19, 2016

Coverage Status

Coverage increased (+0.2%) to 75.342% when pulling f9dd62e on garlick:flux_msg_t into e4a3eac on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2016

Just remembered: this should not be merged until a follow-up PR is submitted to flux-sched. The renaming of flux_msg_get_payload_json and flux_msg_set_payload_json will have broken some code in the simulator.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2016

Hold off merging, I seem to have got changes mixed up in the doc updates I just pushed.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage increased (+0.1%) to 75.338% when pulling ae1ba24 on garlick:flux_msg_t into e4a3eac on flux-framework:master.

@garlick garlick force-pushed the garlick:flux_msg_t branch from ae1ba24 to 63cdcd1 Oct 20, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2016

Sorted out a botched rebase in those doc commits and forced a push. If travis passes, I'm happy with this PR.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage increased (+0.08%) to 75.274% when pulling 63cdcd1 on garlick:flux_msg_t into e4a3eac on flux-framework:master.

@garlick garlick force-pushed the garlick:flux_msg_t branch from 63cdcd1 to cccf0a6 Oct 20, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2016

pushed another doc update there - this time really done :-)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage increased (+0.1%) to 75.319% when pulling cccf0a6 on garlick:flux_msg_t into e4a3eac on flux-framework:master.

garlick added some commits Oct 18, 2016

libflux/message: add flux_msg_frames()
Add an alternative to calling zmsg_size() on a message
to determine the frame count.  Used in testing.
test/tmunge: use flux_msg_t not zmsg_t
Wean test code off of zmsg_t.

Note that this test is not driven by our automated
test framework.  This is because it hasn't been worked out
how to ensure that munge is configured and the daemon running
in those environments.
libflux/message: shorten json accessor names
Rename
  flux_msg_get_payload_json
  flux_msg_set_payload_json
to
  flux_msg_get_json
  flux_msg_set_json

Update the various callers.
libflux/message: add json payload accessors
Add functions that allow JSON message payloads to be
built/parsed using jansson json_pack and json_unpack internally.

flux_msg_vset_jsonf()
flux_msg_set_jsonf()
flux_msg_vget_jsonf()
flux_msg_get_jsonf()
libflux/event: add json payload accessors
Add new event functions that allow json payload to be encoded/
decoded with jansson pack/unpack.

flux_event_encodef()
flux_event_decodef()
libflux/request: add json payload accessors
Add new request function that allow json payload to be
decoded with jansson pack/unpack.

flux_request_decodef()

Decoding a request is common in service implementations
so the convenient payload decoding will have high impact there.
libflux/response: add json payload accessors
Add new response function that allows json payload to be encoded
with jansson pack:

flux_respondf ()

Responding to a request is common in service implementations
so convenient payload encoding will have high impact there.
libflux/rpc: refactor flux_rpcf(), flux_rpc_getf()
Use flux_msg_set_vjsonf() to implement flux_vrpcf()
via internal function rpc_request_vsendf().

Reorder arguments of like functions, rpc_request_send() and
rpc_request_send_raw() to put payload last like rpc_request_vsendf().

Use flux_msg_get_vjsonf() to implement flux_rpc_vgetf().
Drop the internal rpc->rx_json object since the cached decoded
JSON object now belongs to the message.
test/message: cover new payload accessors
In the message unit test, add coverage for the new
flux_msg_get_jsonf() and flux_msg_set_jsonf() functions.
test/event: cover encodef/decodef functions
In the event unit test, add coverage for the new
flux_event_encodef() and flux_event_decodef()
functions.
test/request: cover flux_request_decodef
In the request unit test, add coverage for
flux_request_decodef().
test/rpc: add coverage for new service functions
Enhance the rpc loop test (driven from sharness) to cover
the new flux_request_decodef() and flux_respondf() functions.
In addition use the semi-new flux_rpcf() and flux_rpc_getf()
functions.
doc/flux_respond: add flux_respondf() description
Add a description of the new flux_respondf() function to
the flux_respond(3) man page.

The flux_respond* series of functions are important because
they are typically used when implementing a flux service.
doc/flux_rpc(3): add jansson API reference
Add a citation to the jansson API, which describes the
json_pack() and json_unpack() functions that flux_rpcf()
and flux_rpc_getf() were built from.

Clean up some formatting, making variables italic instead of
bold for consistency.
doc/flux_request_decode(3): new
Add a new man page that covers new and old API functions
that are likely to be needed when writing a "service":

flux_request_decode
flux_request_decodef
flux_request_decode_raw

Reference the jansson manual which describes the variable
argument format for "decodef".
doc/flux_event_decode(3): new
Add a new man page that covers new and old API functions
for manipulating events.

flux_event_decode
flux_event_decodef
flux_event_encode
flux_event_encodef

Reference the jansson manual sections which describe the variable
argument format for "encodef" and "decodef".
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2016

Getting rid of snoop -l broke the generic-utils sharness test. Fixed and forced a push.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage increased (+0.08%) to 75.43% when pulling 61ffe2c on garlick:flux_msg_t into 0eb8cdc on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 20, 2016

Looks good!

@grondo grondo merged commit 62c69e1 into flux-framework:master Oct 20, 2016

4 checks passed

codecov/patch 81.60% of diff hit (target 71.73%)
Details
codecov/project 71.82% (+0.09%) compared to 0eb8cdc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.08%) to 75.43%
Details

@grondo grondo removed the review label Oct 20, 2016

@garlick garlick deleted the garlick:flux_msg_t branch Oct 20, 2016

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.