Fix issues in IPC using TCP sockets (PR #24858)
JP Mattia
This PR addresses several of the IPC issues raised in #24639. It is a substantial rewrite of sckipc.cpp, so my attempt at writing "atomic commits" may fall short. It may be more productive to look at the entire file, along with the individual commit messages and these notes:
-
wxIPCMessageBaselargely replacesIPCStreams.IPCStreamswas burying errors, and was not detecting loss of data sync. Both of these issues are fixed withwxIPCMessageBase. -
For each
IPCCode, there is a message derived fromwxIPCMessageBase. The derived message has methodsDataFromSocket()andDataToSocket(), which simplifies verifying the symmetry in reading and writing. -
Reading and writing messages to the socket are guarded by critical sections, which ensures that a message is read or written to the socket without interruption (and therefore corruption) from another thread.
-
When the socket sends notification, there are potentially multiple messages waiting. The previous implementation would not process more than one message per notification. To fix this, the new
Client_OnRequestimplements a read-and-execute loop, which continues until there is no data left in the socket. A critical section ensures thatClient_OnRequestis alone in receiving messages, and locks out others likeRequest()to avoid race conditions. -
When a method expects a reply (such as
wxConnection::Request()), a critical section prevents any new request until the current request is answered, which ensures that the reply correctly matches the request. Any non-requested messages sent before the reply (eg IPC_ADVISE) are processed, and the read loop continues until the desired reply type is found. -
In the issue thread for #24639, it was mentioned that
ERROR_ACCESS_DENIED(5) and/orWSAECONNABORTED(10053) were being triggered on the socket for wxMSW. I've determined these errors result from attempting to read (or peek) on a socket which has no data waiting. Microsoft, in its infinite wisdom, has determined that trying to read data on a socket that has no data deserves bothERROR_ACCESS_DENIEDandWSAECONNABORTED, which sound much more dramatic than just reading (or peeking) on the socket with no queued data. As such, I've moved those errors into the category ofwxSOCKET_WOULDBLOCK, rather thanwxSOCKET_IOERR; Otherwise, the communication drops at the first attempted read of an empty socket. -
sockmsw.cpp was also filtering out some
FD_READnotifications, and this was causing some hangs in the IPC reads. That filter has been eliminated. A side effect is that notifications from sockmsw.cpp will now sometimes be notifying when the socket does not have queued data waiting. I confess I do not understand why anFD_READnotification is sent in such circumstances, but again, who am I to question the infinite wisdom of Microsoft. -
Data returned from something like
wxConnection::Request()had a potential issue if the caller tried to use the data after anotherwxConnection::Request()had been received. There was only one buffer, and the new request-reply would overwrite the first returned data. The new design creates an array, so that data and the allocations have a lifetime greater than one call.
I have tested the changes successfully with both the ipc sample as well as our company's new windows-service + client. Our service+client has loops which are sending Advise() and Request() back and forth with greater speed than the ipc sample. (Testing with our service also uncovered issue #24856.)
This PR strikes me as a large change, so let me know of any questions and any recommendations as to how to best proceed.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24858
Commit Summary
- baf612c Remove IPCStreams and IPCOutput, ie the IPC interface to the socket.
- dbf2b75 wxIPCMessageBase serves as the new socket interface.
- 7e9c0f5 Create the IPC messages to be passed between processes.
- 5887dfe Utility: Ensure deletion of an IPCMessage when it goes out of scope.
- c08aa6e Fill in the remaining wxIPCMessageBase r/w operations.
- 029d053 Read, write, and peek primitives for comm with the socket.
- 852a0d9 The old Client_OnRequest combined reading and executing. Remove it.
- 7da25b9 The old Server_OnRequest combined reading and executing. Remove it.
- fe1a40b New version of Client_OnRequest.
- 24fe19c ExecuteMessage dispatches based on the IPCCode, and does the work.
- 2b457cc FindMessage is used when a reply is expected for a sent message.
- 2b15645 Server_OnRequest rewritten to incorporate wxIPCMessage
- 0ae566d Utilities for sending fail message, getting wxTCPConnection.
- 2ff6a32 Create an array to track memory allocation.
- 54e67ec Constant values. Note lack of use for one IPCCode.
- 2cc077c Rewrite MakeConnection using wxIPCMessage.
- 539dc6e Rewrite Disconnect. Eliminate stray streams code.
- e11dc93 Rewrite sending of IPC_Execute message.
- 976b60f Rewrite sending of IPC_REQUEST.
- 4f52685 Rewrite sending of IPC_POKE message.
- da7ad62 Rewrite of IPC_ADVISE_START, IPC_ADVISE_STOP, and IPC_ADVISE
- c43c55e Moving/rewriting HandleDisconnect.
- 4765721 We need a pointer to the handler in the connection object.
- 1d21fa7 Accessor for retrieval of the handler.
- befd23d Make WSAECONNABORTED and ERROR_ACCESS_DENIED non-fatal.
- 818944d Stop throwing away socket read notifications.
- 7ea5336 add missing forward definition
- 6e25088 remove stray wxLogMessage calls
- ff07b78 Merge branch 'wxWidgets:master' into jpmattia/issue_24639
File Changes
(4 files)
- M include/wx/sckipc.h (8)
- M include/wx/socket.h (2)
- M src/common/sckipc.cpp (1572)
- M src/msw/sockmsw.cpp (15)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24858.patch
- https://github.com/wxWidgets/wxWidgets/pull/24858.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 467879c eliminate unused vars
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 2 commits.
- d6be4f5 Create an automated test for IPC using sockets.
- 66b36ba Makefile.in that was used to make the sckipc test.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
Update for the latest commits: I've added a test for IPC. There was some previous test code, which was disabled via #ifdef.
Some notes on the new test:
-
For the socket reads and writes to work, IPC needs two separate processes, not just two threads in the same process. This test implements an external process for the server, and then the main testing thread makes calls to that external server.
-
Catch macros will not work in the external process, so the wxIPC
Request()method is used at the end of some tests fetch information from the server and then verify its state. -
ipc.cppis mostly rewritten, so it's probably more productive to read the new file directly rather than a number of incremental commits. Same for the new external server file sckipc_server.cpp -
There are two variables in the beginning of ipc.cpp which are useful for development and manual testing.
g_use_external_server = trueturns on use of the external server. It can be set to false, in which case thetest_sckipc_servershould be started via CLI (or under gdb, for debugging.)g_show_message_timing = truecauses the messages to be printed tostd::cout, which gives you a sense of the message arrival times, and how they are interleaving when multiple threads are running. It defaults tofalse, ie quiet mode so as not to produce output during the automated tests. -
The file
ipc_setup_test.his configured to allow only sockets for IPC. I have not tried it with DDE. -
I am not familiar with bakefiles: There needs to be a modification of
test.bklto implement compilation ofsckipc_server.cpp, since it needs to compile totest_sckipc_server$(EXE). I've added a commit with thetests/Makefile.inthat I've been using for development, but I know that is the incorrect way to do it, and I'm hoping you'll be able to translate that into the necessary bakefile changes. -
This has been a bunch of work, so I've updated authorship on the changed files. I hope that's OK.
All of the tests run successfully on wxMSW, but the most stringent test fails on wxGTK: IPC::AdviseAndRequestMultiThread. Specifically, it fails under Rocky9 linux running in VirtualBox.
I don't have a good handle on why this test fails on Linux. Since the skcipc code succeeds in stress-testing on Windows (setting MESSAGE_ITERATIONS to 5000, and running it 10 times), I'm beginning to suspect something in the linux code (wxSocketImplUnix etc) is causing hanging problems in a multithreaded environment. That will be the first place I'll dig when I have time, but let me know if there might be a better place to check.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 71b3bf1 fix unused-variable error
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 13aa219 fix unused-variable error
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
Hmm, it appears the automated checks are not allowing a separate process to run. For example, under "Ubuntu 18.04 wxGTK 2", the error is execvp(test_sckipc_server) failed with error 2!, and under MSW vs2022 DLL Debug x64 the test is failing on REQUIRE( m_pid != 0 ), which is saying that wxExecute resulted in no pid for the execution.
Any guidance welcome.
PS. Is there any way to run the automated checks without adding to the pull request (so I don't spam your inboxes and can try various things?)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Hmm, it appears the automated checks are not allowing a separate process to run. For example, under "Ubuntu 18.04 wxGTK 2", the error is
execvp(test_sckipc_server) failed with error 2!, and underMSW vs2022 DLL Debug x64the test is failing onREQUIRE( m_pid != 0 ), which is saying that wxExecute resulted in no pid for the execution.Any guidance welcome.
I don't think there is anything preventing the tests from launching other processes, there must be some other problem, e.g. wrong path (especially under Windows, where the executable would fail to run if it can't find the DLLs).
PS. Is there any way to run the automated checks without adding to the pull request (so I don't spam your inboxes and can try various things?)
You can use https://github.com/nektos/act to test Linux workflows locally. And, of course, you can always create a PR to your own repository first, like this the workflows would run there too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
OK, I will have to back-burner this for a while, but I wanted to document the state of the code.
What works:
On wxMSW with mingw64: Configure with
../configure --with-msw --disable-shared --disable-precomp-headers --build=x86_64-pc-mingw64
Running tests IPC* will be error free.
On Unix: Configure with
../configure -enable-debug --disable-shared --disable-precomp-headers
Running tests IPC* will be error free (but note that one test is omitted, see below.)
Some of the github workflows with testing for Unix (ci.yml) will pass.
What needs attention:
The final test IPC::AdviseAndRequestMultiThread does not pass on Unix; It is omitted via #ifdef wxMSW. As mentioned in a comment above, I believe there is a multithreading bug somewhere in the wxSocket code on Unix, because this test passes on Windows. The Unix sockets code would benefit from multithread test coverage, so writing this test would be my first step in solving this issue.
The github workflow for unix fails when --enable-monolithic is turned on. This failure is likely related to issue #24909. As documented in that issue, the wxAppConsole application is (probably) erroneously using the wxGUIEventLoop instead of wxConsoleEventLoop, which might be a bug in the initialization in wxAppTraits, as you noted in the issue.
The github workflow for unix also fails for --with-cxx=20 --enable-utf8 --enable-utf8only. I have not yet investigated this.
The remaining issues revolve around test_sckipc_server(.exe). The IPC tests require two processes, and the test program
has no facility for this, so we use wxExecute to run test_sckipc_server, which serves as the second process.
So to make the github workflows work across all platforms, there needs to be attention devoted to the build process for
- Visual Studio
- cmake
- Xcode
Unfortunately, I don't have sufficient experience with these to build the necessary files; therefore compiling test_sckipc_server to pass the github workflows a stumbling block. I have not modified the bakefile test.bkl, again not something I have experience with.
As you mentioned in #24639, very few people use wxIPC at all and weren't surprised it was broken. (Although I'd note the fact that it is broken might be the reason it is avoided.) So I suppose it's a possibility to #ifdef 0 out the tests (like it is in the master currently) and merge it, and my sense is that it would be less broken than before. It would be good, however, to get test coverage, both for wxIPC as well as whatever is causing the multi-threading wxSocket bug.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#24858)If, by chance, anyone is interested in fixing wxIPC, any help with reviewing, testing and finishing this PR would be welcome. Unfortunately I don't think I'm going to have time to do it in the observable future.
As an aside, I have a question about why do we need to use multiple processes: couldn't we use multiple threads instead, with the server running in its own one?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)Apologies: I'm still buried, so fixing the tests for the various build issues is not possible for quite a while from me. I will note that we have been using this branch of wxIPC for several months in alpha testing, and so far it's good.
As an aside, I have a question about why do we need to use multiple processes: couldn't we use multiple threads instead, with the server running in its own one?
I don't believe it's possible for the same reason that all wxIPC testing had to be removed in the master branch: I originally mentioned in #24639 (comment)
The wxSocketImpl callbacks for the server socket and the client socket cannot run simultaneously in the same process. Because there is only one TCP port in play, whatever last callback installed will have priority, and therefore only the server or the client (but not both) will receive the notification for reading the socket, and only one will get access to the data in the socket buffer.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#24858)Apologies: I'm still buried, so fixing the tests for the various build issues is not possible for quite a while from me. I will note that we have been using this branch of wxIPC for several months in alpha testing, and so far it's good.
This is good to know, but we just can't merge this if it breaks CI :-(
As an aside, I have a question about why do we need to use multiple processes: couldn't we use multiple threads instead, with the server running in its own one?
I don't believe it's possible for the same reason that all wxIPC testing had to be removed in the master branch: I originally mentioned in #24639 (comment)
The wxSocketImpl callbacks for the server socket and the client socket cannot run simultaneously in the same process. Because there is only one TCP port in play, whatever last callback installed will have priority, and therefore only the server or the client (but not both) will receive the notification for reading the socket, and only one will get access to the data in the socket buffer.
Sorry, I don't understand at all: why couldn't the server and the client use different ports?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)This is good to know, but we just can't merge this if it breaks CI :-(
I hesitate to mention this, but testing in the original wxIPC implementation was removed via #if 0, ie it passed CI by not doing any testing. I don't like that solution obviously, and it is likely why there were bugs in the implementation that went unfixed for a long time.
why couldn't the server and the client use different ports?
Ya know, they probably could. I took a quick look back, and as near as I can tell I was simply copying the master branch as much as possible and took it as a given that the same port should be used. It will add a bunch of work in combining the testing files, and I can't guarantee that nothing will be missed by choosing a multithread model over a multiprocess model for testing, but it sure sounds sensible.
[I think the real work will eventually be in figuring out why IPC::AdviseAndRequestMultiThread failed under unix testing. I removed it via #ifdef wxMSW, but there is something squirrelly going on, my best guess was a multithreading wxSocket bug. I've been known to be wrong though. :) ]
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 4 commits.
- 2067d1a Remove external process for test server, use threads instead
- cf8bbe4 fix thread clean up
- 065da16 Fix IPC socket races on Linux
- eaaab85 remove debugging code and msgs
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 5965bc9 CI: Fix unused symbol error
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- b71569a CI: Guard IPC server entry point to main test binary only.
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 31cba66 Fix IPC test teardown use-after-free on pending socket events.
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 35 commits.
- 82adf6c Remove IPCStreams and IPCOutput, ie the IPC interface to the socket.
- 4ffe2e7 wxIPCMessageBase serves as the new socket interface.
- 4e5fe6b Create the IPC messages to be passed between processes.
- 085d513 Utility: Ensure deletion of an IPCMessage when it goes out of scope.
- 03166dc Fill in the remaining wxIPCMessageBase r/w operations.
- df38929 Read, write, and peek primitives for comm with the socket.
- 2b6baed The old Client_OnRequest combined reading and executing. Remove it.
- 396c349 The old Server_OnRequest combined reading and executing. Remove it.
- c9d8a56 New version of Client_OnRequest.
- b570a62 ExecuteMessage dispatches based on the IPCCode, and does the work.
- b81942e FindMessage is used when a reply is expected for a sent message.
- 96c1117 Server_OnRequest rewritten to incorporate wxIPCMessage
- 8a9e304 Utilities for sending fail message, getting wxTCPConnection.
- d37e2d0 Create an array to track memory allocation.
- e82a958 Constant values. Note lack of use for one IPCCode.
- 207c8fd Rewrite MakeConnection using wxIPCMessage.
- e7cdd87 Rewrite Disconnect. Eliminate stray streams code.
- 82cceb1 Rewrite sending of IPC_Execute message.
- e2b3c9d Rewrite sending of IPC_REQUEST.
- 6ff20f9 Rewrite sending of IPC_POKE message.
- a1f4ed0 Rewrite of IPC_ADVISE_START, IPC_ADVISE_STOP, and IPC_ADVISE
- 519954a Moving/rewriting HandleDisconnect.
- 7047a09 We need a pointer to the handler in the connection object.
- ce0014c Accessor for retrieval of the handler.
- 0bfa227 Make WSAECONNABORTED and ERROR_ACCESS_DENIED non-fatal.
- 62c6c01 Stop throwing away socket read notifications.
- 76e927e Fix IPC socket races on Linux
- 9eca92b Fix use-after-free of destroyed IPC socket on the shared event handler
- 0b17a59 sckipc: always re-post wxSOCKET_INPUT after an off-loop message read
- ead0495 sckipc: avoid undefined downcast of a freed socket in Client_OnRequest
- 7db86d0 sckipc: marshal worker-thread IPC socket I/O to the main thread
- 443ce16 sckipc: fix the two-request deadlock by pumping while waiting on the main thread
- 07383ae sckipc: prevent re-entrant socket reads under the MSW GUI event loop
- 12b64b9 sckipc: bound the connection attempt by wxIPCTimeout
- b4345fa Add a multithreaded test for IPC over sockets
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 6ddfa0a Add a multithreaded test for IPC over sockets
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)Picking this back up after a long detour. The short version: your pushback (vadz) on the two-process test structure was the nudge that got me to rework the harness once I had time. I kept a separate server process, but it's now just a re-exec of the same test binary rather than a separate executable, which is what cleared the build blocker. With that plus the threading fix below, the original build/threading blocker is resolved, and the test suite passes on the GUI and console builds across the supported toolchains, with one documented exception I'll cover below. (The Unix builds check is currently red, but that is the Ubuntu 18.04 "Set up build system" step failing across many open PRs right now. It looks like an EOL-container apt/setup issue in the CI infrastructure, not a build or test failure on this branch.)
The Unix/macOS failure was a genuine threading bug, and it is now fixed. Back in 2024 I suspected "something in the linux wxSocket code is causing hanging problems in a multithreaded environment"; that turned out to be on the right track. The 2024 design let worker threads do their socket I/O directly. On wxMSW that's harmless because notifications come through a main-thread, message-based mechanism, which is why it always passed there. On Unix/macOS, event-driven sockets deliver and re-arm through the process-global wxFDIODispatcher that the main loop iterates inside epoll_wait/kqueue, so worker-thread I/O was mutating that shared dispatcher concurrently with the main loop. That's the race behind IPC::AdviseAndRequestMultiThread (and the macOS ModifyFD "modifying unregistered handler?" assert).
The fix is to marshal all socket I/O to the main thread: worker transactions hand their work to the main thread (run inline if already there, otherwise CallAfter() + block the worker on a semaphore), so exactly one thread ever touches the fd, which is the same thread that owns it for event monitoring. The earlier Linux-specific band-aids are reverted now that the root cause is gone. It's contained in sckipc.cpp plus a test dispatch helper. I might flag this as a commit worth reading as a whole rather than as a diff.
On the multiple-processes question: the actual blocker wasn't the multi-process model per se. The second process really needed its own executable (test_sckipc_server), which I couldn't feasibly wire into bakefile/VS/cmake/Xcode. That's gone, and now the server is started by re-executing the same test binary with WX_IPC_TEST_SERVER set, so there's no second executable to build. The client still runs in the main Catch2 process and queries the server for state to verify it (Catch2 macros can't run in the server). The bakefile and generated build files are properly regenerated, and the test now also runs under test_gui.
The one documented test exclusion is wxQt (via __WXQT__). While testing I found that a cross-thread CallAfter() isn't reliably processed by the wxQt event loop (wxQtEventLoopBase::WakeUp() wakes the loop without posting a Qt event), which stalls server-pushed Advise(). That's a wxQt event-loop bug, not a wxIPC bug; I'm finishing a minimal repro and a fix on a separate branch, which I plan to send as its own PR down the road.
Other notes:
- The original implementation "passed CI" by
#if 0, ie. not testing anything. There's real multithreaded coverage now, and it runs clean under TSan, ASAN, and UBSAN. - macOS is the platform that drove a bunch of this work, and it's now green on CI: the wxOSX (Xcode and Ninja), wxMac Intel C++17 (the exact job whose multithread case used to fail), wxiOS, and Universal C++14 builds all pass.
- This PR removes the
wxIPCSocketStreamslayer entirely, replaced bywxIPCMessageBase. It supersedes the recent read-validation work in #26628: the new reader (wxIPCMessageBase) validates the read length inReadSizeAndData()/VerifyLastReadCount().
Also worth mentioning: this branch has been running in production in our windows_service application since the alpha I mentioned earlier, not just in testing.
I force-pushed a curated, logically-grouped history onto this branch (the discussion above is preserved). The early commits walk through the streams-to-messages restructuring step-by-step as a reading aid. They're meant to be read in sequence and don't each compile on their own. The code builds once the message interface and its handler accessor are in place (commit ce0014c), and the functional milestones after that (the marshal rewrite, the two-request fix, the connect-timeout bound, and the multithreaded test) are self-contained and compile on their own. If you'd rather every commit build for bisect, I'm happy to squash the early restructuring into a single commit, since it is broken up only for expository purposes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 13 commits.
- 573ab0d Simplify checks for using the test in "IPC server" mode
- cc4c408 Extract showing information about the test in a separate function
- d891f32 Don't show test information when running it as IPC server
- 5ddbef5 Use <define> instead of <cxxflags> in the test bakefile
- bd49122 Let compiler generate wxTCPServer ctor
- 6303822 Use std::unordered_set instead of set when order doesn't matter
- 114b700 Remove unnecessary wxIPCMessageBase dtor definition
- a6dc949 Replace wxASSERT_MSG with subsequent test with wxCHECK_MSG
- 5e5041f Remove code for unsupported wxUSE_UNICODE==0 build
- 7c0ba5a Modernize the code to use C++11 constexpr and ranged-for
- 9b2b51b Avoid dynamic casts from the type to itself
- 3a836e6 Style fixes to TCP IPC code
- 450dc15 Stop using manual memory management for wxSockAddress
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Sorry, I wanted to merge this before 3.3.3 but there are just too many changes here and I'm too afraid to do it, so I'll wait until the release is done before doing it.
In the meanwhile I've pushed some fixes I did while reviewing this, hopefully you don't mind -- but please let me know if you do.
Concerning the meat of the changes, I like the introduction of per IPC code classes, but I wonder if there is a way to avoid dynamic casts? It looks like we should be sure of the type of the object we have for the given code, and dynamic casts are always suspicious. I'm not sure, but if we could get rid of them, it would be great. TIA!
> #include <stdlib.h> #include <stdio.h> #include <errno.h> +#include <set>
I've replaced all sets with unordered_set as it doesn't seem like we rely on them being sorted anywhere and the latter should normally be used in this case.
> @@ -67,6 +85,11 @@ enum IPCCode
IPC_MAX
};
+// A random header, which is used to detect a loss-of-sync on the IPC
+// data stream. The header is 24-bits, and the IPCCode above is sent in the
+// last 8 bits.
+const wxUint32 IPCCodeHeader=0x439d9600;
Changed this and a few others to be constexpr because why not.
> @@ -75,6 +98,18 @@ enum IPCCode
#include <sys/stat.h>
#endif // __UNIX_LIKE__
+#define wxNO_RETURN_MESSAGE nullptr
+
+const long wxIPCTimeout = 10; // socket timeout, in seconds
+
+
+// For IPC returning a char* buffer. wxWidgets docs say that the user is not
+// supposed to free the memory. Each buffer pointer is assigned to a list
+// sequentially, and the buffer memory is not freed until MAX_MSG_BUFFERS have
+// been assigned.
+#define MAX_MSG_BUFFERS 2048
Isn't 2048 a bit excessive? What is the typical number of buffers used? I'd wager it's much less than this...
> @@ -75,6 +98,18 @@ enum IPCCode
#include <sys/stat.h>
#endif // __UNIX_LIKE__
+#define wxNO_RETURN_MESSAGE nullptr
Also changed this one to be a constexpr variable instead of a macro.
> }
+ virtual ~wxIPCMessageBase() {};
Not really needed, removed.
> {
- Flush();
- return m_dataIn.Read32();
+ if (m_socket && m_socket->IsOk() && m_socket->IsConnected() ) return true;
Reformatted to not put the compound statement on the same line.
> + wxASSERT_MSG( m_handler, "No handler for read allocation"); + if ( !m_handler ) + return false;
Replaced with wxCHECK.
> } + IPCCode m_ipc_code;
Renamed to use the standard camelCase convention (same for the other member variables of this class).
> - unsigned char msg = streams->Read8(); +#if wxUSE_UNICODE
Removed, this is always true now.
> + const wxString& serverName,
+ const wxString& topic)
+{
+ wxSockAddress *addr = GetAddressFromName(serverName, host);
+ if ( !addr )
+ return nullptr;
+
+ wxSocketClient * const client = new wxSocketClient(wxSOCKET_WAITALL);
+
+ // Bound the connection attempt (the TCP connect and the topic handshake
+ // that follows) by the IPC timeout instead of leaving it at the socket's
+ // long default: without this, connecting to a listener that is not yet
+ // ready to complete the handshake blocks for the default timeout (ten
+ // minutes) rather than failing promptly so the caller can retry. The
+ // per-connection socket timeout is set to the same value once connected.
+ client->SetTimeout(wxIPCTimeout);
Hmm, isn't 10s still quite long?
> +
+ // Send topic name, and enquire whether this has succeeded
+ wxIPCMessageConnect msg(client, topic);
+ if ( !handler->WriteMessageToSocket(msg) )
+ {
+ client->Destroy();
+ return nullptr;
+ }
+
+ wxIPCMessageBase* msg_reply = handler->ReadMessageFromSocket(client);
+ wxIPCMessageBaseLocker lock(msg_reply);
+
+ // OK! Confirmation.
+ if ( msg_reply->GetIPCCode() == IPC_CONNECT )
+ {
+ wxTCPConnection *
This doesn't seem to make sense in combination with the dynamic cast below. I've changed this to be wxConnectionBase*.
> +wxTCPServer::wxTCPServer()
+ : wxServerBase()
+{
+ m_server = nullptr;
+}
+
This can/should be just = default.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- acd78f2 Work around gcc 4.8 bug after changes of last commit
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 3 commits.
- 96f5bbd Avoid dynamic casts of IPC messages to their concrete types
- 2338fd5 Always join IPC test server advise workers
- 08fbd05 Fix IPC compilation with g++ 4.8
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)Thanks for the review and the fixes; no objection to any of them, and no problem waiting until after 3.3.3.
As you can imagine, the rewrite wasn't a completely linear process. I kept trying to get away with minimal changes to the original implementation (I failed), and some now-unnecessary code survived in places. Thanks for cleaning those up.
Dynamic casts
Agreed, and they're now gone. The concrete type of a message read from the socket is uniquely determined by its IPC code: ReadMessageFromSocket() is the only producer, and it creates the object via GetIPCMessageFromCode(),
which maps each code to its class. So every cast performed after checking GetIPCCode() could not fail, and the dynamic_cast (plus its dead "cast failed" error branch) was noise.
I replaced them with a small helper, wxIPCMessageCast<T>(), which is a plain static_cast but still verifies the code/type invariant with a wxASSERT in debug builds. Nine casts replaced; the redundant C-cast/null-check/dynamic-cast stack in OnSocketConnection() collapsed to one line.
Two wxDynamicCasts remain, deliberately: the results of OnMakeConnection() / OnAcceptConnection(). Those are user-overridable virtuals returning wxConnectionBase*, so user code can return a type not derived from wxTCPConnection; that check is genuinely dynamic (and existed in the old implementation too).
One thing I noticed while doing this, in "Avoid dynamic casts from the type to itself": in wxTCPClient::MakeConnection() the failure branch does delete connection, but with the cast moved into the if declaration, connection is necessarily null there, so the object returned by OnMakeConnection() leaks. Fixed to delete connectionBase (the server-side counterpart in OnSocketConnection() already had it right).
The gcc 4.8 CI failure
The "Ubuntu 18.04 wxGTK 3 static with gcc 4.8" job was still failing after the "Work around gcc 4.8 bug" commit: that took care of the control-reaches-end complaint, but the primary errors are the unique_ptr derived-to-base returns in GetAddressFromName(). It seems that g++ 4.8 treats a returned local as an rvalue only when its type matches the return type exactly, so the conversion to unique_ptr<wxSockAddress> has to be explicit. A bare return std::move(addr) doesn't work either: it trips -Wredundant-move in the C++20 CI builds. Constructing the return type explicitly satisfies both, and that's what I've pushed.
The 10s timeout
That one is actually your number :-) It came from the original issue discussion: #24639 (comment) ("We definitely should reduce the timeout, I think 10 seconds would be more than enough"), replacing the old 10-minute socket default. In our application 1 second works fine, and I'm happy with whatever default you think is sensible. Longer term I'd prefer it to be a settable parameter (get/set on the connection), but I didn't want to add API items that aren't already in the current documentation. If you'd like it settable, I can do that as a follow-up.
MAX_MSG_BUFFERS
This is accounting for a genuinely awkward issue in the existing API: some calls (like Request(), OnRequest()) hand the caller a pointer to a data block, and as near as I can tell there's no constraint or guidance on the memory management of the returned block. The docs say only that the user must not free it. In truth the user should copy it immediately, as wxIPC was originally written (imho it would have been better if the caller owned the memory, but that ship sailed long ago). So the buffer ring is a tradeoff: returned pointers stay valid until MAX_MSG_BUFFERS further messages have been processed. Our application is request-heavy and 2048 has proven fine, but there's nothing scientific about the number, so I'm open to suggestions for a "best" size, and it could reasonably be smaller.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#24858)Thanks for the review and the fixes
Thanks for cleaning up after my cleanup :-/
Dynamic casts
Agreed, and they're now gone.
I haven't had time to look at the new code yet, but this is great to hear, thanks!
Two
wxDynamicCasts remain, deliberately: the results ofOnMakeConnection()/OnAcceptConnection(). Those are user-overridable virtuals returningwxConnectionBase*, so user code can return a type not derived fromwxTCPConnection; that check is genuinely dynamic (and existed in the old implementation too).
Makes sense.
One thing I noticed while doing this, in "Avoid dynamic casts from the type to itself": in
wxTCPClient::MakeConnection()the failure branch doesdelete connection, but with the cast moved into theifdeclaration,connectionis necessarily null there, so the object returned byOnMakeConnection()leaks. Fixed todelete connectionBase(the server-side counterpart inOnSocketConnection()already had it right).
Oops. Thanks again!
The gcc 4.8 CI failure
The "Ubuntu 18.04 wxGTK 3 static with gcc 4.8" job was still failing after the "Work around gcc 4.8 bug" commit: that took care of the control-reaches-end complaint, but the primary errors are the
unique_ptrderived-to-base returns inGetAddressFromName(). It seems that g++ 4.8 treats a returned local as an rvalue only when its type matches the return type exactly, so the conversion tounique_ptr<wxSockAddress>has to be explicit. A barereturn std::move(addr)doesn't work either: it trips-Wredundant-movein the C++20 CI builds. Constructing the return type explicitly satisfies both, and that's what I've pushed.
I did see that my fix didn't fix it but just didn't have time to get back to it, thanks for dealing with this!
The 10s timeout
That one is actually your number :-) It came from the original issue discussion: #24639 (comment) ("We definitely should reduce the timeout, I think 10 seconds would be more than enough"), replacing the old 10-minute socket default. In our application 1 second works fine, and I'm happy with whatever default you think is sensible. Longer term I'd prefer it to be a settable parameter (get/set on the connection), but I didn't want to add API items that aren't already in the current documentation. If you'd like it settable, I can do that as a follow-up.
I completely forgot about this, of course, but I still think that 10s is more than enough :-) Maybe let's set it to 5s by default, which is still quite conservative? I think setting it globally (and not per connection) would be good enough, but maybe we can merge this first and add it later.
MAX_MSG_BUFFERS
This is accounting for a genuinely awkward issue in the existing API: some calls (like
Request(),OnRequest()) hand the caller a pointer to a data block, and as near as I can tell there's no constraint or guidance on the memory management of the returned block.
I think it's only valid until the next IPC call. At least, looking at DDE-based implementation it definitely seems like it just uses the same buffer for everything.
The docs say only that the user must not free it. In truth the user should copy it immediately,
Yes, I think it's correct.
as wxIPC was originally written (imho it would have been better if the caller owned the memory, but that ship sailed long ago). So the buffer ring is a tradeoff: returned pointers stay valid until
MAX_MSG_BUFFERSfurther messages have been processed. Our application is request-heavy and 2048 has proven fine, but there's nothing scientific about the number, so I'm open to suggestions for a "best" size, and it could reasonably be smaller.
I believe the best size would be 1. Would it be a problem for your application (which is quite probably the most sophisticated consumer of this API in existence)?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 2 commits.
- 56a6af1 Fix use-after-free in IPC test fixture when server startup times out
- 41379db Reduce IPC socket timeout to 5 seconds
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)Before the buffer question, let me push back gently on the parenthetical: "quite probably the most sophisticated consumer of this API in existence." I think that oversells our app and undersells wxIPC: We're not particularly exotic in our use of wxWidgets, and it's probably a natural thing wxIPC invites you to do if you're already using wxWidgets and have a need for IPC.
The history: we've built on wxWidgets for a couple of decades, mostly a Windows GUI app. Over time our customers kept pushing us to run it as a service (headless, with nobody logged in) plus a thin client to see a live view into the service. Since we were already depending on wxWidgets for the graphics, wxIPC was an obvious choice for the service-client link. In principle that's exactly right. So "sophisticated consumer" mostly means "consumer who happened to hit the sharp edges first. In practice it needed more work than we bargained for: the first 90% was making it actually work under Windows, and the next 90% was making it pass CI so we could make a contribution to the project.
All this matters for the buffer question, because it goes to why we're multithreaded in the first place, which is really the crux here: Having both Request and Advise is genuinely great, and it was part of wxIPC's attraction. But between them they guarantee asynchronous behavior. Advise callbacks arrive whenever the server has something to say, and a Request round-trip can take arbitrarily long over a socket. The original design almost certainly assumed everything runs on the main thread, and in a pure single-threaded, one-call-at-a-time world that assumption does tidily solve the buffer-lifetime problem: there's only ever one call in flight, so one buffer is enough.
But blocking the GUI on a socket Request doesn't produce a responsive application; the whole UI freezes for the duration of every round-trip (especially bad if computation is involved for the server result). That's why our requests get dispatched to worker threads: the main loop stays live and paints, and the workers block on the IPC.
looking at the DDE-based implementation it definitely seems like it just uses the same buffer for everything
I haven't delved into DDE (please don't make me do that), but it then has exactly the single-threaded assumption baked in.
I believe the best size [for MAX_MSG_BUFFERS) would be 1. Would it be a problem for your application
Yes, and it's a data race / use-after-free for any multithreaded user.
Running the IPC test suite with MAX_MSG_BUFFERS set to various values shows the issue (using -fsanitize=thread, 5 runs at each, Rocky 10 VM under Win10 host):
| MAX_MSG_BUFFERS | avg races / run | result (5 runs each) |
|---|---|---|
| 1 | 4.8 | race every run |
| 2 | 4.0 | race every run |
| 4 | 1.6 | race every run |
| 8 | 2.0 | race every run |
| 16 | 0 | clean (5/5) |
| 32 | 0 | clean (5/5) |
| 64 | 0 | clean (5/5) |
The tiny sizes race reliably. The exact point where TSan stops reporting is timing-dependent, not a hard number: TSan runs the instrumented code roughly 20x slower per operation, which stretches the window in which a worker is still reading its buffer while the main loop cycles the ring, so the precise cliff shifts with timing, hardware, and load. What's solid is the qualitative result: single-digit buffers are unsafe, and because a descheduled worker can let an unbounded burst of messages cycle the ring before it finishes reading, the "safe" size can't really be pinned down in advance. That's exactly why I'd keep comfortable margin rather than trust a small number that happens to pass today.
So while my 2048 is still unscientific, I think very small numbers are not suitable. I think I'm uncomfortable with numbers below 64.
I'll also note that the genuinely clean fix is to change ownership of the memory, but if that happens it should be at some later time than this PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 54 commits.
- 287cc42 Remove IPCStreams and IPCOutput, ie the IPC interface to the socket.
- ae8b373 wxIPCMessageBase serves as the new socket interface.
- f33dd90 Create the IPC messages to be passed between processes.
- 6b0c344 Utility: Ensure deletion of an IPCMessage when it goes out of scope.
- 16646f2 Fill in the remaining wxIPCMessageBase r/w operations.
- 53fcc32 Read, write, and peek primitives for comm with the socket.
- a896609 The old Client_OnRequest combined reading and executing. Remove it.
- 50dd944 The old Server_OnRequest combined reading and executing. Remove it.
- 97678bb New version of Client_OnRequest.
- b29b296 ExecuteMessage dispatches based on the IPCCode, and does the work.
- 3d944a3 FindMessage is used when a reply is expected for a sent message.
- d445720 Server_OnRequest rewritten to incorporate wxIPCMessage
- cf8770e Utilities for sending fail message, getting wxTCPConnection.
- c8202e3 Create an array to track memory allocation.
- 5e0d6ed Constant values. Note lack of use for one IPCCode.
- 2a1c02e Rewrite MakeConnection using wxIPCMessage.
- c1bac42 Rewrite Disconnect. Eliminate stray streams code.
- f3ece5a Rewrite sending of IPC_Execute message.
- 97f4429 Rewrite sending of IPC_REQUEST.
- 68a1754 Rewrite sending of IPC_POKE message.
- 36950e8 Rewrite of IPC_ADVISE_START, IPC_ADVISE_STOP, and IPC_ADVISE
- 0a32834 Moving/rewriting HandleDisconnect.
- c9cd8fb We need a pointer to the handler in the connection object.
- 8bd68bf Accessor for retrieval of the handler.
- 7e54d32 Make WSAECONNABORTED and ERROR_ACCESS_DENIED non-fatal.
- d293a00 Stop throwing away socket read notifications.
- 21f8859 Fix IPC socket races on Linux
- d22fcfe Fix use-after-free of destroyed IPC socket on the shared event handler
- 7299670 sckipc: always re-post wxSOCKET_INPUT after an off-loop message read
- be2930b sckipc: avoid undefined downcast of a freed socket in Client_OnRequest
- 46ea00f sckipc: marshal worker-thread IPC socket I/O to the main thread
- 069a440 sckipc: fix the two-request deadlock by pumping while waiting on the main thread
- 96357ab sckipc: prevent re-entrant socket reads under the MSW GUI event loop
- 90a9968 sckipc: bound the connection attempt by wxIPCTimeout
- 5dd0985 Add a multithreaded test for IPC over sockets
- 9724edd Simplify checks for using the test in "IPC server" mode
- fadbfa1 Extract showing information about the test in a separate function
- 00062f1 Don't show test information when running it as IPC server
- 332aa5f Use <define> instead of <cxxflags> in the test bakefile
- b1c0580 Let compiler generate wxTCPServer ctor
- 6d39a61 Use std::unordered_set instead of set when order doesn't matter
- 5b460ed Remove unnecessary wxIPCMessageBase dtor definition
- 1634851 Replace wxASSERT_MSG with subsequent test with wxCHECK_MSG
- 15b5c32 Remove code for unsupported wxUSE_UNICODE==0 build
- b68e81a Modernize the code to use C++11 constexpr and ranged-for
- 2c34cf5 Avoid dynamic casts from the type to itself
- f798233 Style fixes to TCP IPC code
- 556240e Stop using manual memory management for wxSockAddress
- d500ec5 Work around gcc 4.8 bug after changes of last commit
- e9a80e4 Avoid dynamic casts of IPC messages to their concrete types
- 3811e5a Always join IPC test server advise workers
- 7277d47 Fix IPC compilation with g++ 4.8
- 44ff0aa Fix use-after-free in IPC test fixture when server startup times out
- 694bf13 Reduce IPC socket timeout to 5 seconds
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#24858)Sorry, I won't be able to look at this/think about it again for some time, but I definitely forgot about the while "multi-threaded" part when I thought about using a single buffer. However, in the last hope of making this idea work, could it still work if we made the buffer per-thread (i.e. a thread-specific variable)?
If not, I think we really need to extend the API to allow using caller-provided buffers and document that the current API only works on the main thread. But having a hardcoded number of buffers with undetermined lifetime just can't be a good idea...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 60529f8 Replace IPC buffer ring with per-thread reply buffers
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)could it still work if we made the buffer per-thread (i.e. a thread-specific variable)?
Yes, that works nicely, and I've pushed the change. The returned buffer now lives in a thread-specific variable, so MAX_MSG_BUFFERS and the buffer ring are gone entirely, and there is no arbitrary number left to pick.
The lifetimes are now deterministic:
- The pointer returned by
Request()stays valid until the nextRequest()call on the same thread. Each thread holds its own last reply buffer, so concurrent requests from different threads cannot invalidate each other's data. - The data passed to
OnExecute()/OnPoke()/OnAdvise()is owned by the message object and is valid for the duration of the callback. That is slightly tighter than the old ring, which kept the buffer alive until it wrapped, but the docs never promised anything longer, and the test suite is unaffected.
One portability note: the thread-specific holder steals the workaround for the MinGW thread_local bug in UntranslatedStringHolder (translation.cpp), incorporated from #26587. So it can be added to the same bucket of things to be removed whenever older MinGW support goes away
TSan (which isn't covered by CI I think) and ASan both run clean over the IPC test suite with this change.
Hopefully that takes care of the last open item. Thanks for the per-thread suggestion, it turned out to be a nice simplification. From my side the PR is ready whenever you have time to come back to it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#24858)Thanks for the updates!
I wanted to make a few cosmetic changes and merge it (after another round of CI) but I ran into a reproducible (although not 100%) bug:
% ./tests/test '[ipc]'
Test program for wxWidgets non-GUI features
build: 3.3.4 (wchar_t,compiler with C++ ABI compatible with gcc 4,STL containers,compatible with 3.2)
compiled using gcc 14.2
running under Debian GNU/Linux 13 (trixie), 6.12.63+deb13-amd64 x86_64 as zeitlin
Filters: [ipc]
IPC WATCHDOG: concurrent main- and worker-thread Request() on the same connection did not complete within 30000 ms; aborting to avoid a CI hang.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test is a Catch v2.13.10 host application.
Run with -? for options
-------------------------------------------------------------------------------
IPC::ConcurrentMainAndWorkerRequest
-------------------------------------------------------------------------------
tests/net/ipc.cpp:1036
...............................................................................
tests/net/ipc.cpp:1036: FAILED:
{Unknown expression after the reported line}
due to a fatal error condition:
SIGABRT - Abort (abnormal termination) signal
===============================================================================
test cases: 10 | 9 passed | 1 failed
assertions: 908 | 907 passed | 1 failed
01:05:08: Debug: Reply failed for IPC Request
01:05:08: Debug: Failed to send IPC_FAIL message: Reply failed for IPC Request
[2] 1819333 IOT instruction ./tests/test '[ipc]'
Any idea about what could be wrong here? This is a pretty standard build, my configure arguments are just --enable-debug LDFLAGS=-fuse-ld=mold.
Also, on the topic of the tests, they take quite a long time to execute (> 1 minute). Could we reduce their execution time without sacrificing too much? Perhaps have some way to run a quick (or, on the contrary, slow) version of these tests while running the full (or minimal) set of them by default?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 6855f80 Disable Nagle algorithm on IPC connection sockets
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)OK thanks for the report. I can reproduce it, and what the watchdog caught is slow behavior (not a deadlock, which was why that watchdog was put in place originally). The trigger is an inefficiency in the IPC code, which causes the test on a slower or loaded machine to go past the 30s limit. The fix is pretty simple: TCP_NODELAY should be set on the socket.
wxIPC was not setting TCP_NODELAY, and each IPC message goes out as several small writes followed by a read of the reply. Without TCP_NODELAY, later writes are held until the first is ACKed, and the peer's delayed ACK sits on that ACK, so each message stalls for tens of ms. (Search for TCP_NODELAY or the Nagle algorithm for details.) Measured here, a Request() round trip on loopback takes ~85ms. That inflated round trip is what slows the worker: a main-thread Request() holds the connection's reply lock for the whole round trip. The test's main loop frees it for only ~5ms per cycle. Mutexes make no fairness guarantee, so the worker's wait per acquisition has no upper bound, only a distribution. On a slow enough machine the worker's iterations go past 30s and the watchdog fires; the timing dependence is why it reproduces "although not 100%". I think your log shows the slowing behavior: 907/908 assertions passed means the main thread ping-ponged cleanly for the full 30s while the worker waited.
About the 30s watchdog: it was added while ironing out the test to make deadlocks fail fast, and I think keeping it is a good idea since it found this issue. A looser bound would have let your run pass, just slowly, and hidden the defect. In truth, because the worker's wait is a distribution with no bound, no finite timeout is provably safe; the watchdog verifies "completes promptly", ie more than "no deadlock". With the fix in, the worst acquisition I can measure under full CPU load is ~88ms, so 30s now has roughly 300x margin.
As for test duration: this is mostly the same TCP_NODELAY problem. The full [ipc] suite here went from 26s to ~8s with the fix; I would expect a similar factor on your machine. Most of the remaining time is the deliberate 50ms pacing between iterations in the multithreaded tests (it exists to interleave Advise() traffic with Request()s rather than have them complete back-to-back). If ~8s is still more than you want in the default run, I can add a quick/full switch, but it may be worth re-measuring first now that the packet-packing stalls are gone.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
- da763f4 Don't use separate timeout for sanitizer builds in IPC tests
- 7cc6ca9 fixup! Add a multithreaded test for IPC over sockets
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#24858)Thanks for the update!
I confirm that disabling Nagle makes things much faster and there are no timeouts any more. 8s is still a bit more than I'd like to, but I guess we can live with that, so I think it should be ready to merge.
I reformatted/renamed variables in the code to follow wx coding conventions in the last commit I pushed (there is also another one which just simplifies the code as I don't see any need to have all these extra checks), please let me know if you see anything wrong with it and if you don't, whether you're fine with applying it as a fixup (i.e. merging with your commit), in which case I'll force push again to do this, or if you'd prefer to keep it separate (which would make it easier for you to merge this with your local branch), in which case I'll still force push but just to change the commit message for the last commit.
Thanks again!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
@jpmattia pushed 1 commit.
- 03a3e70 fixup! Add a multithreaded test for IPC over sockets
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)Glad to hear the fix works for you too. Squashing as a fixup is fine, no need from my side to keep it separate.
I reviewed and tested both commits here, and they look fine, including the timeout unification.
One small thing I noticed in the rename: on the server side, the per-thread counters track Request() serial numbers (they feed HandleThreadRequestCounting() and the get_threadN_request_counter queries), so m_threadAdviseLastVal is a misnomer there; the client side is where the counters track Advise() messages. I pushed a small fixup renaming them to m_threadRequestLastVal, and folded in two other trivial items I spotted while re-reading: RequestMultiThread checked thread2.m_error twice and thread3.m_error never, and a "1, 2, or3" typo in two error messages. Feel free to squash it along with yours.
Thanks for the review and the cleanup!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#24858)(No changes, just force pushed after rebasing the fixups, will merge soon)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JP Mattia
jpmattia left a comment (wxWidgets/wxWidgets#24858)Thanks for merging, and for all the careful review along the way; the code ended up much better for it. If anything IPC-related turns up once this gets wider use, ping me and I'll do my best to address it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#24858)Thanks again to you for all your work here!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.