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

[WIP] DialogueListenableFuture #1545

Closed
wants to merge 1 commit into from
Closed

Conversation

jkozlowski
Copy link
Contributor

Before this PR

This PR is complete garbage, implementation wise, but is a conversation starter.

What if our leaf futures started a context (that has a single useful method #failureCallback) which allows us to add callbacks that should run, should the future fail in any way. The leaf future that returns the Response from the network layer, simply add the Response#close as a callback to the future it returns.

Then all transformation futures (transform/transformAsync/catchingAll) simply need to share this context and propagate it up the chain. If any future fails, we close the context. Closing the context means all the callbacks get run, and forgotten (you can't run #close multiple times).

Subtleties:

  1. What happens when context is already closed: I currently think the callback should run immediately to not leak, but where does the close exception go if it happens?
  2. This is obviously a simplistic solution which assumes there are no forks in the chain of futures: so there's isn't a transformAsync applied twice where the returned futures do completely different things and append new failureCallbacks. In this implementation failure anywhere along either fork would close all resources. I feel like that's probably an assumption we can make for our codebase, implementing forking of contexts sounds not fun.

To make this more specific, we could instead have DialogueResponseFuture, so this is only tied to responses, and not e.g. closing timers or any other stuff.

@carterkozak
Copy link
Contributor

Have you seen ClosingFuture? We requested the feature here: google/guava#3975 however it's not a clean migration, and would almost certainly result in friction (both bugs and api changes) to roll out.

@jkozlowski
Copy link
Contributor Author

Boom! great, haven't seen that

@jkozlowski jkozlowski closed this Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants