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

Allow async methods on worker proxy #7088

Merged
merged 9 commits into from
Aug 15, 2024
Merged

Allow async methods on worker proxy #7088

merged 9 commits into from
Aug 15, 2024

Conversation

matmarchand
Copy link
Contributor

@matmarchand matmarchand commented Aug 14, 2024

Adding async support to enable worker proxy so we can run async operation like fetch(). In my case, it is a large geojson stream that would like to avoid moving from the the main thread to the worker thread.

@pmconne
Copy link
Member

pmconne commented Aug 14, 2024

Please explain use case.
Please write a test that confirms that when you do:

await workerProxy.someLongRunningAsyncOperation();
await workerProxy.someFastSynchronousOperation();

the first operation always completes before the second.

matmarchand and others added 2 commits August 14, 2024 17:54
Co-authored-by: Paul Connelly <22944042+pmconne@users.noreply.github.com>
@matmarchand
Copy link
Contributor Author

Please explain use case. Please write a test that confirms that when you do:

await workerProxy.someLongRunningAsyncOperation();
await workerProxy.someFastSynchronousOperation();

the first operation always completes before the second.

Could you explain what you have in mind with this test? javascript 'await' contract is "to wait for a promise and get its fulfillment value" so the second line will never execute before the first one has ended.

@pmconne
Copy link
Member

pmconne commented Aug 15, 2024

javascript 'await' contract is "to wait for a promise and get its fulfillment value" so the second line will never execute before the first one has ended.

No, it doesn't block the JS event loop, it yields to the JS event loop.
This means that, for example, onmessage can be invoked again while you're awaiting your promise.
The message queue built into WorkerProxy should prevent onmessage from being reinvoked until the first operation completes. The test is supposed to prove that it actually does.

I asked about use cases because in some cases it may be important that operations execute in sequence - e.g., "first pass some data to be processed, then invoke an operation to process that data". In others, it may not matter (e.g., "perform this atomic operation and return the result"). Yours sounds like the latter?

@matmarchand
Copy link
Contributor Author

matmarchand commented Aug 15, 2024

javascript 'await' contract is "to wait for a promise and get its fulfillment value" so the second line will never execute before the first one has ended.

No, it doesn't block the JS event loop, it yields to the JS event loop. This means that, for example, onmessage can be invoked again while you're awaiting your promise. The message queue built into WorkerProxy should prevent onmessage from being reinvoked until the first operation completes. The test is supposed to prove that it actually does.

I asked about use cases because in some cases it may be important that operations execute in sequence - e.g., "first pass some data to be processed, then invoke an operation to process that data". In others, it may not matter (e.g., "perform this atomic operation and return the result"). Yours sounds like the latter?

From now, it's the latter, I'm expecting to make atomic requests so sequence is not important. I updated the test and the message queue does not seem to produce a FIFO result.

@pmconne
Copy link
Member

pmconne commented Aug 15, 2024

I updated the test and the message queue does not seem to produce a FIFO result.

You're testing the opposite of what I suggested, which is fine, but different.
I'll update the tests.

@matmarchand matmarchand marked this pull request as ready for review August 15, 2024 19:01
@pmconne pmconne enabled auto-merge (squash) August 15, 2024 19:02
@pmconne pmconne merged commit de15d99 into master Aug 15, 2024
16 checks passed
@pmconne pmconne deleted the MathieuM/asyncWorker branch August 15, 2024 19:33
naronchen pushed a commit that referenced this pull request Sep 27, 2024
Co-authored-by: Mathieu Marchand <matmarchand@users.noreply.github.com>
Co-authored-by: Paul Connelly <22944042+pmconne@users.noreply.github.com>
naronchen pushed a commit that referenced this pull request Sep 30, 2024
Co-authored-by: Mathieu Marchand <matmarchand@users.noreply.github.com>
Co-authored-by: Paul Connelly <22944042+pmconne@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.

2 participants