Skip to content

Comments

Reject promises for in-progress operations on an MLTensor if it is destroyed#799

Merged
fdwr merged 4 commits intowebmachinelearning:mainfrom
a-sully:destroy-rejects-pending-promises
Mar 19, 2025
Merged

Reject promises for in-progress operations on an MLTensor if it is destroyed#799
fdwr merged 4 commits intowebmachinelearning:mainfrom
a-sully:destroy-rejects-pending-promises

Conversation

@a-sully
Copy link
Contributor

@a-sully a-sully commented Dec 13, 2024

This PR handles the following case:

const promise = context.readTensor(tensor);
// Destroy `tensor` before awaiting `promise`
tensor.destroy();  // This should reject

The current spec does not reject promise in this example. This PR updates the spec to match the Chromium implementation and what I assume to be the desired behavior (please speak up otherwise!)

Note that Chromium implements a similar pattern in other cases where the MLContext may be destroyed (either intentionally or unexpectedly), but specification of that work should follow #744


Preview | Diff

Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

LGTM with one nitpick

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM!

@fdwr
Copy link
Collaborator

fdwr commented Jan 30, 2025

FYI @bbernhar and @RafaelCintron.

@anssiko anssiko requested a review from RafaelCintron March 6, 2025 08:50
Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

👍

@a-sully
Copy link
Contributor Author

a-sully commented Mar 13, 2025

Just resolved merge conflicts. Should be ready to go 👍

@fdwr fdwr merged commit 4c8c75e into webmachinelearning:main Mar 19, 2025
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 19, 2025
…stroyed (#799)

SHA: 4c8c75e
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants