Remove extra lock-taking in preopen setup #491
Conversation
Issue [#8392] in Wasmtime highlights how preopen registration can result in a hang when compiled for a threaded environment. The problem is that `internal_register_preopened_fd_unlocked` assumes it will not touch the global lock because its caller, `__wasilibc_populate_preopens`, already has taken the lock. Unfortunately, a refactoring in WebAssembly#408 (which introduces `internal_register_preopened_fd_unlocked`) did not catch that the `resize` function called internally also takes the global lock. This change removes that locking in `resize` under the assumption that it will only be called when the lock is already taken. [#8392]: bytecodealliance/wasmtime#8392
|
I would like to verify that this is correct somehow: it would be nice to run |
|
@alexcrichton, is there a way I can "plug in" an external wasi-libc sysroot to [edit:] never mind, I see you explained this elsewhere! |
|
Ok, I confirmed by building a new |
|
If this change is not released, the resize function in the existing version 22 will end up waiting indefinitely for a lock. I think this is quite an important fix. Is there any plan for an urgent release? |
IIRC the wasi threads effort is kind of on hold, pending shared everything threads. Are you using it production somehow / somewhere despite that? |
|
I'm not opposed releasing a v22.1 but, like @sbc100 mentioned, with how experimental wasi-threads is it likely does not need to be urgent (?). |
Yes I use threads as my game, ui engine. I didn't know that wasi threads is the pending one. Should I have to stop using wasi thread? |
That is up to you. I don't know which engines (if any) support wasi threads so if you choose to depend on wasi-threads you would likely be severely limiting your portability. |
|
that's my own engine, implemented threads on rust wasm32-wasip1-threads target with web worker. It's somewhat difficult to understand. I think threads are a very important feature for performance in WASI as well, but could you explain why there seems to be little interest in supporting them? Is there a roadmap that I am not aware of, or should I assume that the WebAssembly Team is not interested in multithreading in WASI? |
|
There is very much interest in multi-threading in Wasm and in WASI. Currently work is ongoing on the shared-everything threading proposal, and my understanding is that most of the WASI efforts are now focused on building on top of that: https://github.com/WebAssembly/shared-everything-threads. @abrown I wonder if we can add something to this effect in the readme for https://github.com/WebAssembly/wasi-threads? |
|
Indeed the shared-everything threading proposal is the long-term approach, however, there are teams (like mine) that currently use wasi-threads (through WAMR) and due to the limited options for updating the code, will have to keep it around for at least 5 years. We currently use Rust so we don't depend on the wasi-sdk release (at least for now) as Rust stdlib uses wasi-libc directly. |
|
Sorry I didn't mean to block the release of new version of wasi-sdk. I think the bar for cutting new releases should be very low (assuming the steps involved are not cumbersome, but if they are we should fix that). |
|
@loganek, do you want to take a stab at creating a new v22.1 release following this guide? |
|
@namse, do you want to test out the new |
|
Wonderful! I've just tested it and problem fixed. Thank you ^_______^ @abrown |
Issue #8392 in Wasmtime highlights how preopen registration can result in a hang when compiled for a threaded environment. The problem is that
internal_register_preopened_fd_unlockedassumes it will not touch the global lock because its caller,__wasilibc_populate_preopens, already has taken the lock. Unfortunately, a refactoring in #408 (which introducesinternal_register_preopened_fd_unlocked) did not catch that theresizefunction called internally also takes the global lock. This change removes that locking inresizeunder the assumption that it will only be called when the lock is already taken.