Fix crash on simultaneous IPC calls using the same thread #214

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.

Testing

I have added a unit test in the last commit. You can cherry-pick this commit on master to reproduce the crash.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• // Use callFnAsync() to get the client to setup the request_thread that will be used for the test. -> ... to set up the request_thread that will be used for the test. [“setup” is a noun; the verb form should be “set up”.]
• // NOTE: '3' was choosen because it was the lowest number -> // NOTE: '3' was chosen because it was the lowest number [“choosen” is a misspelling of “chosen”.]

^{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.

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.

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.

@ryanofsky I was inspired by your comment to try a different approach in https://github.com/bitcoin-core/libmultiprocess/pull/214/commits/1238170f68e8fa7ae41c79465df5cdae34d568e9 to make the test more reliable by setting 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 = 3 value

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.