Relevant Issue: #206
This error occurs when non-C++ clients make simultaneous IPC calls to the server using the same Server thread.
The libmultiprocess C++ client ensures a 1-to-1 mapping of client-to-server threads, so simultaneous calls using the same thread cannot be made.
I have added a unit test in the last commit. You can cherry-pick this commit on master to reproduce the crash.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Possible typos and grammar issues:
drahtbot_id_5_m
Nice fix! And the python test looks like it could be a nice addition: https://github.com/bitcoin-core/libmultiprocess/compare/master...Eunovo:libmultiprocess:test-ipc-with-py
I do think it should be pretty straightforward to write a c++ unit test too, not sure if you were planning on doing that or if something was in the way.
Nice fix! And the python test looks like it could be a nice addition: master…Eunovo:libmultiprocess:test-ipc-with-py
I wasn’t sure it was worth it to add the Python test. It could be useful for reproducing scenarios experienced by non-libmultiprocess clients in general, but I’m not convinced.
I do think it should be pretty straightforward to write a c++ unit test too, not sure if you were planning on doing that or if something was in the way.
I think it might be possible if I use the Capnp library directly instead of the libmultiprocess client.
I think it might be possible if I use the Capnp library directly instead of the libmultiprocess client.
Yes that would be the idea. You should be able to modify existing tests which connect with the libmultiprocess client, and use the ProxyClient<messages::FooInterface> it object it provides. But instead of calling libmultiprocess methods directly on that object, call capnproto methods on the object’s messages::FooInterface::Client m_client member.
This error occurs when non-libmultiprocess clients make simultaenous IPC calls to the server using the same Server thread.
The libmultiprocess Cpp client ensures a 1-to-1 mapping of client-to-server threads, so simultaneous calls using the same thread cannot be made.
Yes that would be the idea. You should be able to modify existing tests which connect with the libmultiprocess client, and use the
ProxyClient<messages::FooInterface>it object it provides. But instead of calling libmultiprocess methods directly on that object, call capnproto methods on the object’smessages::FooInterface::Client m_clientmember.
Done
Code review 21886f2bbdf7238282909f04c8c0d57ec4f75726. Thanks for the new test!
The approach in the test seems pretty clever. In order to trigger the “thread busy” error, you need the server IPC thread to be busy waiting for something when a new request comes in. By having the server run the “callbackSaved” method, it guarantees the thread will be busy because it will be stuck trying to call the client while the event loop is occupied.
I think a more direct approach is probably possible, maybe by adding new FooInterface wait and stopWaiting methods where wait just blocks indefinitely until stopWaiting is called, and the “thread busy” error can be called by just making 2 wait requests simultaneously. Or instead of adding any new methods, the existing callFnAsync method can be called instead of wait() and m_fn could just be set in the beginning of the test to wait indefinitely until the end of the test.
I do like current approach though and there’s no need to change it.
Although the libmultiprocess client prevents this kind of situation from occuring, it easily occurs in non-libmultiprocess clients
m_fn to wait on a promise. It worked, but it still needs 3 simultaneous calls. I expected this approach to work with just 2 calls, but for some reason, m_fn is unset when the second call posts to the waiter.
That’s interesting. 1238170f68e8fa7ae41c79465df5cdae34d568e9 looks like a straightforward test for the problem, and I would also expect only 2 simultaneous calls to necessary not 3.
Code review ACK 1238170f68e8fa7ae41c79465df5cdae34d568e9, since the code changes here all make sense except the initial running = 3 value, and there is a comment about that, so at least it’s called out and may be something that can be improved later.
since the code changes here all make sense except the initial
running = 3value
Figured out the reason running = 3 is required. The Waiter class is actually capable of waiting for one previously posted function to be executed when a new one is posted. It just doesn’t allow a third function to be posted when one is currently executing and one is currently waiting to be executed. So the test needs to send two pending requests to occupy the wait class before “thread busy” will be returned.