Repository navigation
IPC portability - #136
IPC portability#136
Conversation
950b714 to
f27565a
Compare
xyephy
left a comment
xyephy
left a comment
There was a problem hiding this comment.
tACK f27565a
Built and ran test_sv2 on each commit (macOS). On the head commit I also ran the template provider and lifecycle tests repeatedly under TSan and ASan+UBSan.
With the new teardown, RequestInterrupt() sets the flag before Interrupt() is called, so Interrupt() returns early and its body never runs. As a result, no test exercises BlockTemplate::interruptWait() over IPC anymore. When I replace the interruptWait() call in Sv2TemplateProvider::Interrupt() with a no-op, tp_tester_repeated_construction fails on master (teardown waits the full 5s template interval), while the whole suite still passes on this branch.
Could you add a test that calls Interrupt() while a client is in waitNext() and checks that it returns quickly? Such a test only catches this if MockMining::interrupt() doesn't wake waitNext() (added some in line comments).
The header fixes from bitcoin-core/libmultiprocess#370 are already on master via #134. Is anything else blocking this, or is it just waiting for #370 to be merged so it can be picked up in a subtree update?
c8abc52 fixes the dangling client capture. There's a related issue: finished client threads are never joined until shutdown, so client_threads only grows. I'll open a small follow-up for that on top of this PR.
| { | ||
| LogPrintLevel(BCLog::SV2, BCLog::Level::Trace, "mock interrupt()"); | ||
| LOCK(state->m); | ||
| state->shutdown = true; |
There was a problem hiding this comment.
In Bitcoin Core, Mining::interrupt() only interrupts createNewBlock() and waitTipChanged(). waitNext() isn't affected by it: only interruptWait() on that template (or a new tip, a fee rise, the timeout or node shutdown) ends the wait. With this change the mock wakes waitNext() too, so a test that calls Interrupt() can't tell a broken interruptWait() path from a working one. Maybe drop it, or only let it affect createNewBlock()?
| } | ||
|
|
||
| // Signal the mock state directly (bypasses IPC) so that any | ||
| // MockBlockTemplate::waitNext() blocked on the event-loop thread |
There was a problem hiding this comment.
Nit: waitNext takes a Proxy.Context, so it blocks a server worker thread rather than the event-loop thread.
|
|
||
| // Wait for the handler thread before releasing its IPC proxies. | ||
| if (m_tp) { | ||
| m_tp->Interrupt(); |
There was a problem hiding this comment.
This is a no-op after RequestInterrupt(), because Interrupt() returns early once m_flag_interrupt_sv2 is set. That also skips the connman interrupt here (the destructor does it later). Either drop the call or mention it in the RequestInterrupt() doc.
Yes. I'll look into your other feedback as well. |
Avoid capturing a reference to the Sv2Client object in the lambda run by the per-client handler thread. The client iteration happens under ForEachClient and the reference would dangle if the client list mutates while the thread is running. Pass the id by value, as ThreadSv2ClientHandler takes a size_t anyway.
Let tests set the shutdown flag before unblocking mock waitNext() calls, so the handler exits instead of issuing another IPC request.
Interrupt blocked createNewBlock() calls without affecting waitNext(), matching the node mining interface. Keep mining interruption separate from the direct mock shutdown used during fixture teardown. Test both interrupting template creation and stopping a client blocked in waitNext() through interruptWait() over IPC. The latter must finish well before the five-second template interval, without mock shutdown.
Replace the Unix-only socketpair() setup in TPTester with mp::SocketPair(), and store the socket ids in a small std::array<mp::SocketId, 2> initialised with mp::SocketError so the code is valid when SocketId is an unsigned type (as on Windows). Drops <sys/socket.h>/<unistd.h> from the test harness.
Stop the handler before releasing its IPC proxies, and hold the server connection explicitly so teardown can destroy it on the event loop. This avoids depending on socket disconnects to release the loop. Keep all server connection access on the event-loop thread, including the reset after a disconnect may already have destroyed it.
Call SetupNetworking() from sv2-tp's own entry point. On Windows this initializes Winsock for both IPC and SV2 sockets; on Unix it is a no-op.
f27565a to
42feb83
Compare
Extracted from and preparation for #93.