QUIC: Blocking calls are unreliable in multi-threaded usage
@hlandau is already working on this.
Since Apr 16, 2024.
- #24257 by @hlandau — closed without merging
- Dominant language
- C
- Stars
- 30.8k
- Forks
- 11.5k
- Avg merge
- 10m
- Merged PRs (30d)
- 1
Description
Problem
The implementation of blocking calls (e.g. to SSL_read) in the QUIC implementation in OpenSSL 3.2 and 3.3 has a somewhat fundamental conceptual error in the implementation approach which renders blocking calls (SSL_set_blocking_mode(ssl, 1)) unreliable when used with multiple threads performing (auto-)ticking calls on the same connection.
To recap, a blocking call to e.g. SSL_read calls the function block_until_pred in quic_impl.c, which is designed to block until a certain condition is met:
SSL_read(conn0)
block_until_pred()
waits on the network read/write BIOs and a timeout, calls select()/poll()
However, consider the following sequence involving two threads T1 and T2:
T1: connects conn0 and subsidiary streams stream0, stream1
T1: enters blocking call to SSL_read(stream0), calls to OS poll(conn0_fd)
T2: enters a call to SSL_write(stream1) (which happens to not block);
T2: call to stream1 autoticks conn0 (SSL_handle_events(conn0))
T2: recvmsg() receives datagram and processes it before T1's call to poll(2) wakes up
T2: stream0 is now ready to read
T1: still blocking, and will continue to block until event timeout
The underlying issue here is that if a datagram is received on a socket and a thread is currently blocking in a call to poll() on that socket, this does not guarantee that that thread will be woken up, for example if another thread calls recv() on that socket before a context switch occurs.
Resolution
The only viable resolution for this issue is to create another OS resource which can be passed to select()/poll() and use this to artificially wake other threads.[^1][^2]
On Linux, eventfd(2) etc. exists for this purpose. On other platforms, including Windows, this functionality can be emulated using a pipe or socketpair. It is possible to support all of our supported platforms this way.
Essentially, a socketpair or eventfd is made readable by writing a byte to it, causing poll() in other threads to wake. The readability condition is then cleared by reading the byte from the FD. The final solution actually needs to be a fair bit more complicated than this as there is a need to reliably wake up all waiting threads, not just one; the solution will probably look similar to the condition variable emulation code for Windows XP I wrote found in crypto/thread/arch/thread_win.c.
In any case, the problem with this approach is that it requires creating OS socket resources behind the back of the application, which feels at best rude for a library, particularly one like OpenSSL which seeks to allow the application to control all I/O. I intend to find a better story here (e.g. allowing the application to choose between non-blocking usage only, blocking but single-threaded usage, neither of which requires internal sockets to be created, and blocking multi-threaded usage with internal polling sockets created by OpenSSL).
In any case, IMO the solution is too invasive to backport to 3.2 and 3.3 and this will need to be documented as a known issue.
[^1]: At least, in the non-thread-assisted case. When we are allowed to spin up a thread, as in thread-assisted mode, all I/O processing could be moved to the assist thread and a condition variable could then be used instead. Note that there are compelling reasons to do this in many use cases, especially server-side, so this is likely to become a supported model in the future anyway.
[^2]: Technically you can wake a poll() call via a signal which results in EINTR. This is obviously not a sane or portable solution.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.