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

fix(futures): Ensure finish time is set before callback function dispatch occurs #29530

Merged
merged 8 commits into from
Oct 25, 2021

Conversation

tkaemming
Copy link
Contributor

This fixes a subtle sequencing dependency that was inadvertently broken with #28700. The finish time was not available to callback functions, since it was not set prior to Future.set_result being called. (It was available by the time that TimedFuture.set_result in the existing tests.) This maintains the intended behavior (not overriding the finish time in the case of a failure to set the result on a previously finished future) while ensuring the value is also available to callbacks (which are only dispatched in the case that the future properly changes state.) This also extends the tests to cover the callback invocation, and changes the behavior of TimedFuture.set_exception to bring it in line with the behavior of the set_result method.

start_time, finish_time = expected_timing = (1.0, 2.0)

callback_results = []
callback = lambda future: callback_results.append((future.result(), future.get_timing()))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this only works because get_timing intentionally returns a new tuple (for immutability) rather than a reference to the internal data structure.

If there's way that this can be rewritten more elegantly to not require the callback_results local with mocks that would be nice, but I couldn't find anything that tracks the return values of the inner callable (only call arguments.)

Needed Python 3.8 Dockerfile upgrade from b0cfeb1 to avoid failing onpremise build.
@tkaemming
Copy link
Contributor Author

These messages have stopped being logged after deploying this change:

# If either endpoint of the timing data is not set, just ignore this call.
# This really shouldn't happen on the primary backend, but playing it safe
# here out of an abundance of caution.
if not all(primary_timing):
logger.warning(
"Received timing with unexpected endpoint: %r, primary_backend_name: %r, future_status: %r",
primary_timing,
primary_backend_name,
primary_status,
)
return

@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants