Decrement proxying queue refcounts in finally blocks #16571
Conversation
This ensures that the refcounts are correctly decremented even if a proxied task throws an exception, although it won't protect again other bad things like dropped work that could happen in that case.
|
@sbc100 is there a way to extract this common code out into a helper function that will be available both inside library_pthread.js and worker.js? |
|
Aha, it turned out to be straightforward. |
| // Always decrement the ref count. | ||
| Atomics.sub(HEAP32, e.data.queue >> 2, 1); | ||
| } | ||
| executeNotifiedProxyingQueue(e.data.queue); |
There was a problem hiding this comment.
I don't think you can call library functions directly from worker.js can you? I think you would need to make sure the library function was exported on the module and then call Module[' executeNotifiedProxyingQueue']. Ensure its exported and not minimized is kind of tricky... and happens in couple of different places I think (for example MINIMAL_RUNTIME has explicit code to export these on the Module).
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
lgtm % the usage in worker.js
|
Somehow the call to the library function in worker.js succeeds. I verified this by changing its name at the worker.js call site, which caused the tests to fail due to the unknown name. Optimizations don't seem to cause problems either, since all the tests pass. |
The way that the module is injected into the workers namespace is still kind of mystery to me.. and I think the behaviour is different in the browser to in node. So maybe check that test passes when run as a browser test (I know its kind of pain, but I think it should be easy enough use |
This is alarming! |
Yes, I think it stems from the fact that the worker model is different and this effects the way that code gets imported. Luckily it only effects rather small amount of code in worker.js.. most code is not effected. If you are correct and we don't need to specify Module['stringName'] when we call back into module code from the worker, then we can probably update a lot of the code there. |
|
Unfortunately I won't be able to run the browser test until I'm back in the office on April 11 and have access to a desktop development environment again :/ |
|
I can verify this for you.. (you can also use ssh port forwarding if you can ssh into your desktop) |
|
Thanks, that would be great. Unfortunately I don't think ssh port forwarding for graphics works with the ChromeOS SSH app :( I could probably CRD into my workstation, but IIRC it's not set up correctly for that. |
No, I meant port-forward the http server port. For example using Here is what I sometimes do: This will compile the browser test but not try to run it.. then you can browse to port 6931 to run the test in your own tab. |
|
I'm back in the office so I was able to run the browser test directly on my workstation. It worked fine! I'll go ahead and land this. |
This ensures that the refcounts are correctly decremented even if a proxied task
throws an exception, although it won't protect again other bad things like
dropped work that could happen in that case.