Skip to content

Locking while writing to stdout may give a deadlock #10155

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

Closed
cwmos opened this issue Jan 7, 2020 · 3 comments
Closed

Locking while writing to stdout may give a deadlock #10155

cwmos opened this issue Jan 7, 2020 · 3 comments

Comments

@cwmos
Copy link

cwmos commented Jan 7, 2020

When stdout is flushed from a thread in a webworker, it seems like the thread will not continue until the data has been handled by the main thread. This may lead to a deadlock.

Here is a simple example where stdout is used for logging and protected by a lock. Protecting by a lock is necessary in order to avoid corruption of the output - see https://stackoverflow.com/questions/6374264/is-cout-synchronized-thread-safe.

#include <emscripten.h>
#include <thread>
#include <mutex>
#include <iostream>

std::mutex lock;

void myLogFunction(const char* txt) {
  lock.lock();
  std::cout << txt << "\n" << std::flush;
  lock.unlock();
}

extern "C" void EMSCRIPTEN_KEEPALIVE mainFunction() {
  myLogFunction("We are in the main thread");
}

void callFromWorker() {
  MAIN_THREAD_ASYNC_EM_ASM({
    Module._mainFunction();
  });

  myLogFunction("We are in the webworker");
}

int main() {
  new std::thread(callFromWorker);
}

This code deadlocks.

(The code it even hangs chrome in the sense that it is impossible to close the browser tab running the code or the browser itself But I guess this should be fixed in Chrome and likely they are aware of this problem.)

I understand that making stdout asynchronous may give other problems in terms of the webworker producing the output faster than the main thread can consume it leading to memory issues. Still, this may be a better solution than the current one.

@juj
Copy link
Collaborator

juj commented Jan 7, 2020

Yes, this is unfortunately currently expected to deadlock. :/ The bad pattern here is essentially

#include <emscripten.h>
#include <thread>
#include <mutex>
std::mutex lock;
extern "C" void EMSCRIPTEN_KEEPALIVE foo() {
  lock.lock();
  MAIN_THREAD_EM_ASM({}); // Synchronously block to wait for the main thread
  lock.unlock();
}
void worker() {
  MAIN_THREAD_ASYNC_EM_ASM({Module._foo()}); // Make main thread want to call foo()
  foo();
}
int main() {
  new std::thread(worker);
}

Not even -s PROXY_TO_PTHREAD=1 will help here.

The issue here is that fd_write is a synchronous syscall. It needs to be synchronous for two reasons: calling fd_write passes a pointer of data to the function, and there is a return value to observe back from the call.

So what happens is the pthread initiates a sync syscall while holding the lock, so it is waiting for main thread to finish fd_write for it, and main thread attempts to get lock.lock() to satisfy the MAIN_THREAD_ASYNC_EM_ASM block. Note that proxied requests are queued in-order, so the main thread does not even get to process the fd_write call, but the proxied call queue for the main thread has the two entries, the MAIN_THREAD_ASYNC_EM_ASM request, and the fd_write request, and it is stuck processing the first.

The first part could be fixed, there is already a machinery in place for satellite buffers, e.g. WebGL call proxying utilizes this. So the data to be printed could be copied off to a separate satellite data section that travels with the proxied message, and main thread would then free the satellite data.

The second part however about the return value (number of bytes written) cannot be fixed without breaking away from WASI syscall layer.

ASMFS shows one design to fix this problem, as it buffers fully multithreaded without proxying, and only full lines get printed via console.log/console.error in the worker that invokes the call. So printing does not have either sync or async proxying in ASMFS. (Although ASMFS is not yet multithreading safe in general, and unfortunately currently on a hiatus, it regressed when wasi syscalls were implemented, and I have not had time to fix it back)

@cwmos
Copy link
Author

cwmos commented Jan 17, 2020

Thank you for the detailed explanation. I understand there is no obvious way to fix with this. For my use case I ended up just calling console.log directly using EM_ASM in the webworker without sending the log to stdout to the main thread. Perhaps emscripten can solve it in a similar way by letting it be up to the user of emscripten to deal with the stdout data in the webworker. I understand that it may be difficult to find a good place for the user of emscripten to provide this function.

qtprojectorg pushed a commit to qt/qtbase that referenced this issue Nov 4, 2020
emscripten_async_run_in_main_runtime_thread_ schedules
an async call on the on the main thread. However, the
calls are ordered, also in respect to _synchronous_ calls
to the main thread (for example those made during file
write/flush).

Making a synchronous call from a secondary thread may
then cause Emscripten to service previously scheduled
async calls during the synchronous call. This can cause
a deadlock if:

- a secondary thread makes a sync call while holding a lock, and
- a previously scheduled async call attempt to acquire the same
  lock on the main thread.

(See emscripten-core/emscripten#10155
for sample code)

Avoid this case by adding a second zero-timer async call;
this way Qt should process events when the main thread
becomes idle.

Change-Id: I221fe4e25bbb1a56627e63c3d1809e40ccefb030
Pick-to: 5.15
Reviewed-by: Lorn Potter <[email protected]>
@stale
Copy link

stale bot commented Jan 16, 2021

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Jan 16, 2021
@stale stale bot closed this as completed Feb 18, 2021
tobydox pushed a commit to in-hub/qtbase that referenced this issue Feb 23, 2021
emscripten_async_run_in_main_runtime_thread_ schedules
an async call on the on the main thread. However, the
calls are ordered, also in respect to _synchronous_ calls
to the main thread (for example those made during file
write/flush).

Making a synchronous call from a secondary thread may
then cause Emscripten to service previously scheduled
async calls during the synchronous call. This can cause
a deadlock if:

- a secondary thread makes a sync call while holding a lock, and
- a previously scheduled async call attempt to acquire the same
  lock on the main thread.

(See emscripten-core/emscripten#10155
for sample code)

Avoid this case by adding a second zero-timer async call;
this way Qt should process events when the main thread
becomes idle.

Change-Id: I221fe4e25bbb1a56627e63c3d1809e40ccefb030
Pick-to: 5.15
Reviewed-by: Lorn Potter <[email protected]>
(cherry picked from commit 4035fdd)
qtprojectorg pushed a commit to qt/qtbase that referenced this issue Mar 2, 2022
emscripten_async_run_in_main_runtime_thread_ schedules
an async call on the on the main thread. However, the
calls are ordered, also in respect to _synchronous_ calls
to the main thread (for example those made during file
write/flush).

Making a synchronous call from a secondary thread may
then cause Emscripten to service previously scheduled
async calls during the synchronous call. This can cause
a deadlock if:

- a secondary thread makes a sync call while holding a lock, and
- a previously scheduled async call attempt to acquire the same
  lock on the main thread.

(See emscripten-core/emscripten#10155
for sample code)

Avoid this case by adding a second zero-timer async call;
this way Qt should process events when the main thread
becomes idle.

Change-Id: I221fe4e25bbb1a56627e63c3d1809e40ccefb030
Reviewed-by: Lorn Potter <[email protected]>
(cherry picked from commit 4035fdd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants