-
Notifications
You must be signed in to change notification settings - Fork 81
thread-safe spawn with ngx_notify #218
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
base: main
Are you sure you want to change the base?
Conversation
e327e07 to
e1c9191
Compare
1dcea34 to
53f60c3
Compare
schedule() can now be called from any thread and will move tasks to the event loop thread using ngx_notify (ngx_event_actions.notify). This enables receiving I/O notification from "sidecar runtimes" like async-compat, and requires less unsafe. The async example has been rewritten to use async_::spawn, demonstrating usage of reqwest and hyper clients wrapped in Compat to provide a tokio runtime environment while using the async_ Scheduler as executor.
|
I see it now.
If it's guaranteed that all tasks run on the main thread, I don't think it's dangerous. This change only allows scheduling from other threads. It's not uncommon that libraries start their own helper threads, for instance. async-compat starts a transparent tokio runtime in a thread for IO completion handlers, while still using our executor for the tasks. I also can image situations where you'd want to start non-IO compute in a thread pool to not block nginx - in our case, for example, crypto. You'd want to be able to notify the request handler async task of completion by writing to a channel or a similar mechanism. This, in turn, would call the waker from that thread (AFAIK), which calls schedule for the task from that thread, but the woken task would be scheduled to run on the main thread via ngx_notify.
We need to work with the request heavily (mutate headers_in and headers_out, read client bodies, produce response bodies) in response to I/O (external requests, database queries, custom crypto/tunneling), which can only be done on the main thread safely. If all our code is running in a completely separate engine, it all becomes extremely hard. In addition, we need a way to interrupt nginx' epoll reacting I/O events, which aren't all bound to a request (OpenID shared signals, e.g.).
I don't think it would do that. If the waker is invoked from the main thread, schedule in my branch would simply .run() the runnable, and everything stays on the main thread. ngx_notify would not be called (except once during the lifetime of a worker process because it's not known which tid is main). I have to admit I didn't test with nginx-acme yet though. To recap, I'd still like the following:
Given ngx_epoll_module.c:769, ngx_notify from other threads is indeed inherently unsafe. However, what if we do this:
Would this work for you? |
Proposed changes
As mentioned in #110 my POC for async via nginx-notify.
This would enable
schedule()to be called from other threads, e.g. async-compat or other "sidecar-runtime" setups. It also makes sure epoll is interrupted when there are IO completion notifications coming in from outside of the event loop, leading to prompt continuation.While this doesn't provide a native
hyper/clientas @bavshin-f5 wanted, it makes the default tokio implementation work via Compat. This would be a viable stopgap solution for us. I've added some examples, including hyper and reqwest. In the future, one could implement a "sidecar-runtime" approach as in async-compat natively that would use a separate epoll loop in a thread, or inject additional fds from the Rust side to nginx's epoll instance (if possible).Some notes:
ngx_notify, afaik this includes epoll, kqueue, eventport. Thread pools seem to have the same limitation, so this might be finestdas a dependency forasyncto reflect that (this would be a breaking change, but async Rust probably implies std anyways).Checklist
Before creating a PR, run through this checklist and mark each as complete.