Skip to content

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

Merged
fdwr merged 4 commits into
webmachinelearning: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 into
webmachinelearning:mainfrom
a-sully:destroy-rejects-pending-promises

Conversation

@a-sully

@a-sully a-sully commented Dec 13, 2024 •
edited by pr-preview Bot
Loading

Copy link
Copy Markdown
Contributor

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

inexorabletash
inexorabletash approved these changes Dec 13, 2024
View reviewed changes

@inexorabletash inexorabletash left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with one nitpick

Comment thread index.bs Outdated
huningxin
huningxin approved these changes Dec 16, 2024
View reviewed changes

@huningxin huningxin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@fdwr

fdwr commented Jan 30, 2025

Copy link
Copy Markdown
Collaborator

FYI @bbernhar and @RafaelCintron.

@anssiko
anssiko requested a review from RafaelCintron March 6, 2025 08:50
fdwr
fdwr approved these changes Mar 13, 2025
View reviewed changes

@fdwr fdwr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

@a-sully

a-sully commented Mar 13, 2025

Copy link
Copy Markdown
Contributor Author

Just resolved merge conflicts. Should be ready to go 👍

@fdwr
fdwr merged commit 4c8c75e into webmachinelearning:main Mar 19, 2025
github-actions Bot added a commit that referenced this pull request Mar 19, 2025
@a-sully @github-actions
Reject promises for in-progress operations on an MLTensor if it is de…
c4414a4 
…stroyed (#799)

SHA: 4c8c75e
Reason: push, by fdwr

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

Reviewers

@huningxin huningxin huningxin approved these changes
@fdwr fdwr fdwr approved these changes
+2 more reviewers
@inexorabletash inexorabletash inexorabletash approved these changes
@RafaelCintron RafaelCintron RafaelCintron approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@a-sully @fdwr @inexorabletash @huningxin @RafaelCintron