Repository navigation
fix(threading): run message dispatch and library code on one dispatcher thread - #3453
Conversation
579d1fd to
0b23ec4
Compare
| ## How it works | ||
|
|
||
| - **One thread, one item at a time.** The dispatcher thread runs driver messages (`Connection.Dispatch`) and all library code. The transport reads and parses on the thread pool and posts each message to the dispatcher. A connection that `BrowserType.ConnectAsync` creates shares the dispatcher of the connection that owns its pipe. | ||
| - **API calls hop on and off.** `Connection.WrapApiCallAsync` posts the body of a public call to the dispatcher, and returns the result on the thread pool. The caller's continuation never runs on the dispatcher thread, and a `SynchronizationContext` that the caller captured is honored as with any other `Task`. |
There was a problem hiding this comment.
Is it a requirement that "continuation never runs on the dispatcher thread" or just a side effect of the client using a thread pool that is outside of playwright's control? In other words, why not have the client code also run on the same thread and simplify this?
There was a problem hiding this comment.
Client code may have assumptions about threads, and might block, which is common in dotnet. So we don't to run client code on our thread where possible.
| /// </summary> | ||
| internal static class CopyOnWrite | ||
| { | ||
| internal static void Add<T>(ref List<T> list, T item) |
There was a problem hiding this comment.
should it grab a lock on list to ensure there are no 2 concurrent CopyOnWrite.Add calls that would result in only one of them adding an item in the resulting list?
There was a problem hiding this comment.
By definition, all mutators run on a single Dispatcher-owned thread, so cannot run concurrently.
|
|
||
| // The thread is gone. Run the action on the thread pool so that nothing waits forever; | ||
| // after the connection has closed there is no state left to protect. | ||
| ThreadPool.UnsafeQueueUserWorkItem(static state => ((Action)state)(), action); |
There was a problem hiding this comment.
This is weird behavior. Can we just throw in this case?
There was a problem hiding this comment.
Well, we need the action to throw, not the Post method, and it will most likely throw the "Connection is closed" error.
…er thread The client keeps the JS method bodies, which rely on the event loop: nothing happens between a state check and the event subscription that follows it. Each connection now has a dispatcher thread that restores this. - Driver messages and library code run on the dispatcher thread, one item at a time. - An API call hops onto it in WrapApiCallAsync and returns on the thread pool. - Completion sources are completed after the current message, as a JS microtask. - Event accessors replace the handler list atomically on the caller's thread; the dispatcher thread alone tells the server which events to send. - Lists that getters hand out are copy-on-write. See docs/threading.md for the model and the rules. Fixes: microsoft#3364 Fixes: microsoft#3399
0b23ec4 to
6db854d
Compare
661b08e
into
microsoft:main
Summary
WrapApiCallAsyncand returns on the thread pool. Completion sources are completed after the current message, like a JS microtask, soPage.Loadhas fired whenWaitForLoadStateAsyncreturns.docs/threading.mddescribes the model and the rules for contributors.Fixes #3364
Fixes #3399