flux_future_continue(3) can't be used with flux_kvs_lookup()

[grondo]

As noted in #2119 flux_future_continue(3) and the internal fulfill_next() continuation it uses do not have access to the underlying future implementation, so can only propagate a future result to the next future in a chain, and not other flux_future_t components such as the underlying aux hash.

The flux_kvs_lookup(3) implementation stashes a lookup_ctx within a flux_future_t to properly handle flux_kvs_lookup_get(3), and so a future returned from flux_kvs_lookup can not be used with flux_future_continue(3) (nor can any other flux API call which makes use of the aux hash for get functionality)

To fix, some kind of functionality would need to be added to the main future.h API to allow composite_future.c access to the underlying aux hash of a future. Each item in the hash would need to be copied up to the "next" future in the chain, without propagating the destroy methods to avoid double free.

Another approach would be to move the composite and chained future support to a first-class property of flux_future_t, which would allow more flexibility in the implementation, and would avoid exposing flux_future_t internals via the API. However, this would require moving a bit of code, and probably would involve changes to the internal flux_future_t structure, but would likely result in a cleaner implementation in the long run, IMO.

[garlick]

My main concern at the time with integrating composites with the "core" future implementation was I didn't want the complexity to get out of control (and felt it was already a bit beastly), but this might actually be an opportunity to refactor the internals to make it more understandable and serviceable, while fine tuning the interface to be clearer and more consistent.

Maybe we ought to weigh the pros and cons and decide if this is where we want to invest some work in the near term...

[grondo]

Maybe we ought to weigh the pros and cons and decide if this is where we want to invest some work in the near term...

Looking at this further, we probably do not want to invest any time in the near term to fold composite_future.c into future.c. I think we can get a long way if we added functionality to the base future type to fulfill a future with another future. This would do essentially what fulfill_next() does in composite_future.c, but it can have complete functionality since it would have access to future internals.

I'll propose a new function, something like:

/* Fulfill future `f` using the already fulfilled future `p`. This embeds `p` 
 * in the future `f`, copying the fulfilled result or error value into `f`, and
 * gives `f` read-only access to the underlying aux data of `p`.
 * `flux_future_fulfill_with` steals a reference to `p`, which will be destroyed
 *  when `f` is destroyed.
 *
 * Returns 0 on success, -1 on error with errno set:
 *  EINVAL:  either `f` or `p` is NULL, or `f` == `p`
 *  EAGAIN: future `p` is not ready
 */
int flux_future_fulfill_with (flux_future_t *f, flux_future_t *p);

[garlick]

That sounds reasonable to me.

…

On Tue, Apr 16, 2019, 9:22 AM Mark Grondona ***@***.***> wrote: Maybe we ought to weigh the pros and cons and decide if this is where we want to invest some work in the near term... Looking at this further, we probably do not want to invest any time in the near term to fold composite_future.c into future.c. I think we can get a long way if we added functionality to the base future type to fulfill a future with another future. This would do essentially what fulfill_next() does in composite_future.c, but it can have complete functionality since it would have access to future internals. I'll propose a new function, something like: /* Fulfill future `f` using the already fulfilled future `p`. This embeds `p` * in the future `f`, copying the fulfilled result or error value into `f`, and * gives `f` read-only access to the underlying aux data of `p`. * `flux_future_fulfill_with` steals a reference to `p`, which will be destroyed * when `f` is destroyed. * * Returns 0 on success, -1 on error with errno set: * EINVAL: either `f` or `p` is NULL, or `f` == `p` * EAGAIN: future `p` is not ready */int flux_future_fulfill_with (flux_future_t *f, flux_future_t *p); — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2127 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKX2_FRN_Msljdk1mHYGwILtsVwj4gjks5vhfjNgaJpZM4cxTXY> .