Work around macOS 27 startup issues#6978
Conversation
| /* | ||
| QTcpSocket socket; | ||
| socket.connectToHost("localhost", port); | ||
| if (socket.waitForConnected(100)) { | ||
| socket.disconnectFromHost(); | ||
| throw tr("Port %1 is already in use").arg(port); | ||
| } | ||
| */ |
There was a problem hiding this comment.
We should fix this in a different way. The test should be performed on other platforms than macOS. Also, on macOS, we should find another way to check if the port is already in use.
There was a problem hiding this comment.
Thank you for the review.
I agree that simply disabling the check is probably not the best long-term solution and that a platform-specific or more robust implementation would be preferable.
My intention with this patch was mainly to identify the source of the issue and verify that the failure originates from the current localhost connection test.
On macOS 27 Beta, commenting out this pre-check consistently allows Webots to start and operate normally, while the original code reports every tested port as already in use, even when the ports are confirmed to be free.
From my testing, this suggests that the problem is likely located in the current localhost probe rather than in QTcpServer::listen() itself.
I completely agree that the final fix should preserve the port availability check and remain compatible with other platforms.
In the meantime, this workaround may still be useful for affected users running macOS 27 Beta, as it provides a temporary way to bypass what is currently a startup-blocking issue until a more complete solution is implemented.
If it would be useful, I can perform additional tests on macOS 27 Beta and help evaluate alternative approaches for checking port availability on macOS.
There was a problem hiding this comment.
Would a platform-specific approach be acceptable?
For example, we could keep the existing localhost pre-check on non-macOS platforms, but skip it on macOS and rely directly on QTcpServer::listen() there. This would preserve the current behavior on Linux/Windows while avoiding the false-positive localhost connection behavior observed on macOS 27 Beta.
Conceptually:
#ifndef APPLE
QTcpSocket socket;
socket.connectToHost("localhost", port);
if (socket.waitForConnected(100)) {
socket.disconnectFromHost();
throw tr("Port %1 is already in use").arg(port);
}
#endif
mTcpServer = new QTcpServer();
if (!mTcpServer->listen(QHostAddress::Any, port))
throw tr("Cannot set the server in listen mode: %1").arg(mTcpServer->errorString());
This would make the workaround macOS-specific instead of removing the pre-check globally.
There was a problem hiding this comment.
I was investigating this, and I'm now wondering if the check is even necessary. The comment references QAbstractSocket::BindMode. Notably the only flag that that claims to set is SO_REUSEADDR, which looks like it still blocks connections on ports with active servers. The real worry is setting SO_REUSEPORT, which would allow that behavior. So, just going by that, we should be fine. Nevertheless, I had some doubts, so here's my in-depth analysis:
- The default bind behavior only sets the
AddressReusablesocket engine option on UNIX [1] - Same goes for
QTcpServer[2] - This option typically only corresponds to
SO_REUSEADDR[3] (called here) - On Mac, this can sometimes set
SO_REUSEPORT, but only on UDP, which doesn't matter here [4]
On Windows, there are SO_REUSEADDR and SO_EXCLUSIVEADDRUSE. If the former is set, we can get multiple servers on the same socket. If the latter is set (admin required), it prevents use of the former. [5]
- The default bind behavior can set
AddressReusableand/orBindExclusivelybased on the flags. [6] - These correspond to the two windows flags [7] (called here)
QTcpServersets neither explicitly, so if we run by theBindModedefaults (see first link), we'd expectShareAddress, which corresponds to both flags being unset. (see [6])
All of that is to say, just looking at the code, I don't think Qt sets any options which would cause the behavior this check is trying to prevent, so we could try starting the TCP server first and checking the error code. If it fails with an in-use conflict, return the "in use" error. Otherwise, fall back to the generic version. Of course, we should probably check that behavior before merging it in.
Also, is it just me, or does create have a memory leak on the second throw whereby mWebSocketServer and mTcpServer are never deleted? start() looks like it just recursively calls create on the same WbTcpServer object, so the destructor is never called to delete the intermittent members.
There was a problem hiding this comment.
(I am still curious as to why the current behavior is failing on mac though 👀 )
There was a problem hiding this comment.
I think that makes sense. My original intention was mainly to identify the source of the startup failure and provide a temporary workaround. Using the result of QTcpServer::listen() as the source of truth seems cleaner than relying on a localhost connection probe. From my testing on macOS 27 Beta, removing the pre-check consistently restores normal startup behavior, which would also be consistent with the idea that the check itself may no longer be necessary.
There was a problem hiding this comment.
Also, is it just me, or does create have a memory leak on the second throw whereby mWebSocketServer and mTcpServer are never deleted? start() looks like it just recursively calls create on the same WbTcpServer object, so the destructor is never called to delete the intermittent members.
Yes, I believe you are right. The minimal fix is to clean up before throwing (and ideally parent mTcpServer too for symmetry):
mWebSocketServer = new QWebSocketServer("Webots Streaming Server", QWebSocketServer::NonSecureMode, this);
mTcpServer = new QTcpServer(this);
if (!mTcpServer->listen(QHostAddress::Any, port)) {
const QString error = mTcpServer->errorString();
delete mTcpServer; mTcpServer = NULL;
delete mWebSocketServer; mWebSocketServer = NULL;
throw tr("Cannot set the server in listen mode: %1").arg(error);
}This also keeps isActive() (mWebSocketServer != NULL) honest after a failed attempt.
There was a problem hiding this comment.
That makes sense.
I hadn't noticed the cleanup issue, but I agree that deleting the server objects before throwing would avoid leaving the object in a partially initialized state and would keep isActive() consistent after a failed startup attempt.
It also seems compatible with using QTcpServer::listen() as the primary source of truth for port availability.
There was a problem hiding this comment.
Would you like me to update the PR in this direction?
I can replace the current workaround with a cleaner implementation that removes the localhost pre-check, relies on QTcpServer::listen() as the source of truth, and cleans up mTcpServer / mWebSocketServer before throwing if listen() fails.
I’m actually an MSc student rather than a regular contributor to the Webots codebase, so I may not be fully familiar with all the design considerations here. If this direction is consistent with the intended architecture, I’d be happy to implement and test it on macOS 27 Beta lol
There was a problem hiding this comment.
Yes; that would be awesome! Please go-ahead make those changes!
8304748 to
ed510bf
Compare
ed510bf to
93e9de1
Compare
This PR is related to issue #6976.
It contains two small changes that allowed Webots R2025a to build and run on macOS 27 Beta:
In WbTcpServer.cpp, the localhost TCP pre-check is disabled because it falsely reports all ports as already in use on macOS 27 Beta.
In g_image.c, stb_image_write.h is included with quotes instead of angle brackets to satisfy the newer macOS/Clang include behavior.
This has been tested by rebuilding Webots from source on Apple Silicon M1 Max. The patched build launches correctly, loads worlds, and runs controllers.
I understand that the TCP change may be considered a workaround rather than the final upstream fix. I am submitting this PR mainly to make the tested patch visible and easier to review.