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: add flux_future_reset() #1503

Merged
merged 9 commits into from May 3, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented May 3, 2018

This PR adds flux_future_reset(), to "unfulfill" a future.

The driving use case was RPC with multiple response messages. One can now do this:

flux_future_t *f = flux_rpc (...)
do {
    flux_rpc_get (f, ...);
    flux_future_reset (f);
} while (condition);
flux_future_destroy (f);

or in a continuation, one can conditionally call either flux_future_reset() or flux_future_destroy() before returning. If reset, the continuation will be called on the next fulfillment (RPC response, or whatever).

Added unit tests and updated man pages. I reworked the "user" man page a bit for better readability also.

I'd say test coverage might be a little light here, so I'll see how that goes and possibly add more tests.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 3, 2018

Coverage Status

Coverage decreased (-0.01%) to 78.982% when pulling c38b407 on garlick:rpc_multi into b148196 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 3, 2018

Codecov Report

Merging #1503 into master will decrease coverage by <.01%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master    #1503      +/-   ##
==========================================
- Coverage   78.69%   78.68%   -0.01%     
==========================================
  Files         164      164              
  Lines       30673    30683      +10     
==========================================
+ Hits        24139    24144       +5     
- Misses       6534     6539       +5
Impacted Files Coverage Δ
src/common/libflux/rpc.c 92.37% <100%> (-0.19%) ⬇️
src/common/libflux/future.c 88.98% <96.42%> (+0.66%) ⬆️
src/cmd/flux-start.c 74.19% <0%> (-2.51%) ⬇️
src/common/libflux/handle.c 83.66% <0%> (-1%) ⬇️
src/common/libflux/message.c 81.25% <0%> (-0.24%) ⬇️
src/cmd/flux-event.c 69.78% <0%> (ø) ⬆️
src/modules/kvs/kvs.c 66.03% <0%> (+0.16%) ⬆️
src/modules/connector-local/local.c 72.95% <0%> (+0.2%) ⬆️
src/broker/overlay.c 74.13% <0%> (+0.31%) ⬆️
... and 3 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 3, 2018

Coverage looks good to me. Only case that doesn't appeared covered is when the timer_watcher needs to be reset instead of created in then_context_set_timeout:

https://codecov.io/gh/flux-framework/flux-core/pull/1503/diff#D1-194

This actually reminds me of a possible enhancement for flux_future_reset (or maybe this is possible already, I haven't checked). It would make the code cleaner for some multi-response use cases if the timer and/or continuation handler could be reset at the same time the future is reset. This would allow a chain of continuations, possibly with different timeouts, to be used with a multi-response RPC when that makes sense (e.g. continuation 1 creates some state, resets the future to call continuation 2 on next response which uses that state and so on)

Of course this could be done now by keeping a state variable in void * arg to the future, but I think code would be easier to follow in some cases if this wasn't required.

Perhaps this could be a new function flux_future_again with the same prototype as flux_future_then (or I guess flux_future_then called a fulfilled future could reset it? That could be bad design though..)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 3, 2018

This actually reminds me of a possible enhancement for flux_future_reset (or maybe this is possible already, I haven't checked).

Spoke to @garlick and this is already possible, you just have to call flux_future_then after flux_future_reset. Something like flux_future_again might be a nice-to-have to combine these two operations, but since flux_future_reset is just one line, that would probably just be bloat.

One open question was whether flux_future_then after flux_future_reset should be added to the tests in this PR.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 3, 2018

I'll add a unit test for calling flux_future_then() again. As an aside: It shouldn't actually matter what order flux_future_reset() and flux_future_then() are called.

As I think we discussed, flux_future_then() cannot do an implicit reset, since some futures might be fulfilled immediately (say if something is already present in a local cache), and then the reset would cause that "event" to be ignored.

flux_future_again() would be OK, though I wonder if it's that useful given that flux_future_reset() shouldn't require error checking. We could always add it later.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 3, 2018

flux_future_again() would be OK, though I wonder if it's that useful given that flux_future_reset() shouldn't require error checking. We could always add it later.

Yeah, sorry, I came to the same conclusion in my follow up comment.

As an aside: It shouldn't actually matter what order flux_future_reset() and flux_future_then() are called.

Ah good point. I was thinking maybe if the last future result was an error, this code would be triggered if flux_future_reset was not called before flux_future_then:

   if (f->result_errnum_valid) {
        errno = f->result_errnum;
        return -1;
    }
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 3, 2018

Oh indeed you are right about that case of error fulfillment impacting the required order. Thanks!

@garlick garlick force-pushed the garlick:rpc_multi branch 2 times, most recently from 5631b59 to 1245390 May 3, 2018

garlick added some commits Apr 30, 2018

libflux/future: add flux_future_reset()
Problem: there is no way for a future to be "unfulfilled"
in cases where there are multiple events to be handled by
the future.

Add flux_future_reset(), which sets up the future to be
fulfilled again, and invalidates any result currently stored
in the future container.

Allow flux_future_then() to be optionally called again
after flux_future_reset(), to set a timeout or change
the continuation callback.
libflux/rpc: handle multiple responses
To prepare the way for making flux_future_reset()
work on RPC futures to support multiple responses
with the same matchtag:

1) drop the rpc->inflight flag and instead call
flux_future_wait_for (0) to check if the future
is fulfilled before freeing matchtag.

2) Instead of stopping the message handler in the
response callback, leave it running.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 3, 2018

I fixed a bug, fixed a test bug (that was masking the other bug), and added coverage for calling flux_future_then() after flux_future_reset(). The order does matter, and I updated the man page to indicate this.

Then I squashed down the incremental development.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 3, 2018

Great! can this go in?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 3, 2018

Oh, noticed a typo in 1d8f244: serveral

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 3, 2018

Sure!

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 3, 2018

Er wait, typo!

@garlick garlick force-pushed the garlick:rpc_multi branch from 1245390 to c38b407 May 3, 2018

garlick added some commits May 3, 2018

rpc/test: cover multiple responses
Add tests to cover RPCs with multiple responses.

An RPC issues a requested number of responses followed
by an error response with errnum == ENODATA (like EOF).

1) Call flux_rpc_get() in a loop, with flux_future_reset()
after each successful call.  Verify that the requetested
number of responses was received.

2) Set up a continuation that calls flux_future_reset()
after a successful flux_rpc_get().  On failure, verify
that the requested number of responses was received.

3) Like #2 except handle the first response in one
continuation, and subsequent responses in another.
It covers calling flux_future_then() a second time on
a future.
doc/flux_future_then(3): [cleanup] fix typos
Problem: flux_future_wait_for() is shown to return
"int bool" in the man page, and flux_future_check()
(a removed function) is referenced in RETURN VALUE.

Drop the extra "bool".

Drop the reference to flux_future_check().
doc/flux_future_get(3): restructure man page
Problem: flux_future_then(3) doesn't get to the point.

Make the "base" manual page for _using_ futures
flux_future_get(3) instead of flux_future_then(3), which
seems a bit more intuitive.

Add a prominent reference to flux_future_get(3) at
the top of flux_future_create(3), so that one doesn't have
to read very far to realize it's man page for class
builders not users of such classes.
doc: update SEE ALSO refs to flux_future_get(3)
Since flux_future_get(3) is now the "base" future man page,
update references to flux_future_then(3) in the SEE ALSO
section of other man pages, where appropriate.
doc/flux_future_get(3): add flux_future_reset()
Add a description for flux_future_reset() to the
flux_future_get(3) man page.
doc/flux_future_get(3): simplify descriptions
Problem: flux_future_get(3) includes some details that
are not likely to be needed by most users, and this
makes it hard to understand.

Drop some obscure details that are more or less
covered in flux_future_create(3).

@grondo grondo merged commit aa881c9 into flux-framework:master May 3, 2018

4 checks passed

codecov/patch 97.05% of diff hit (target 78.69%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +18.36% compared to b148196
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 78.982%
Details

@garlick garlick deleted the garlick:rpc_multi branch May 3, 2018

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.