Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Correctly call bind only when necessary #16

Merged
merged 2 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/main-qt6.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ jobs:
- name: Build NetUdp Unit Tests
run: cmake --build build --target "NetUdp_Tests" --config "${{ matrix.build_type }}" -j

- name: Build NetUdp QtQuick Unit Tests
run: cmake --build build --target "NetUdp_QuickTests" --config "${{ matrix.build_type }}" -j

- name: Run NetUdp Unit tests
run: cd build && ctest --build-config "${{ matrix.build_type }}" --progress --verbose --parallel 12
if: "!contains(matrix.os, 'ubuntu')"
Expand Down
63 changes: 47 additions & 16 deletions src/NetUdp/Worker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ struct WorkerPrivate
bool inputEnabled = false;
bool separateRxTxSockets = false;

bool validInputConfiguration() const
{
return inputEnabled && rxPort != 0;
}

// ──────── MULTICAST INTERFACE JOIN WATCHER ────────
// Created when at least one iface is being joined. ie (!_p->joinedMulticastGroups.empty() || !_p->failedJoiningMulticastGroup.empty())
// Destroy in 'onStop', when joinedMulticastGroups & failedJoiningMulticastGroup are both empty
Expand Down Expand Up @@ -219,7 +224,7 @@ void Worker::onStart()

if(_p->inputEnabled)
{
if(_p->rxAddress.isEmpty())
if(!_p->rxAddress.isEmpty())
{
qCWarning(netudp_worker_log) << "Start Udp Socket Worker rx : " << _p->rxAddress << ":" << _p->rxPort
<< "tx port :" << _p->txPort;
Expand All @@ -231,7 +236,7 @@ void Worker::onStart()
}
else
{
if(_p->rxAddress.isEmpty())
if(!_p->rxAddress.isEmpty())
{
qCWarning(netudp_worker_log) << "Start Udp Socket Worker rx port : " << _p->rxAddress << ":" << _p->rxPort;
}
Expand Down Expand Up @@ -330,26 +335,50 @@ void Worker::onStart()
// Qt multicast issues are non resolved ? https://forum.qt.io/topic/78090/multicast-issue-possible-bug/17
const auto bindSuccess = [&]()
{
if(useTwoSockets)
return _p->rxSocket->bind(QHostAddress(_p->rxAddress),
_p->rxPort,
QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint)
&& _p->socket->bind(QHostAddress(), _p->txPort, QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint);
auto* const socket = [&]() -> QUdpSocket*
{
if(!_p->validInputConfiguration())
return nullptr;

if(_p->inputEnabled)
return _p->socket->bind(QHostAddress(_p->rxAddress),
_p->rxPort,
QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint);
return _p->socket->bind(QHostAddress(), _p->txPort, QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint);
if(useTwoSockets)
return rxSocket();

return _p->socket;
}();

if(socket)
{
qCDebug(netudp_worker_log) << "Bind to " << _p->rxAddress << ":" << _p->rxPort;

const auto hostAddress = _p->rxAddress.isEmpty() ? QHostAddress(QHostAddress::AnyIPv4) : QHostAddress(_p->rxAddress);

return socket->bind(hostAddress, _p->rxPort, QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint);
}

qCDebug(netudp_worker_log) << "No need to bind to any address";
return false;
}();

qCDebug(netudp_worker_log) << "Bind success : " << bindSuccess << ", valid input configuration : " << _p->validInputConfiguration();

// Finish the start of the socket
// Or start watchdog on failure
if(bindSuccess)
if(bindSuccess || !_p->validInputConfiguration())
{
qCDebug(netudp_worker_log) << "Success bind to " << _p->socket->localAddress() << ":" << _p->socket->localPort();
// Sorry for this atrocity, but this is how the library public API is designed
// Library is in maintenance mode, this highlight bad design and library should be reworked
if(!_p->validInputConfiguration())
{
_p->isBounded = true;
Q_EMIT isBoundedChanged(true);
}

if(!_p->multicastGroups.empty() && rxSocket() && _p->inputEnabled)
if(bindSuccess)
{
qCDebug(netudp_worker_log) << "Success bind to " << _p->socket->localAddress() << ":" << _p->socket->localPort();
}

if(!_p->multicastGroups.empty() && rxSocket() && _p->validInputConfiguration())
{
// Join multicast groups either on every ifaces or in '_p->incomingMulticastInterfaces' given by user
if(_p->incomingMulticastInterfaces.empty())
Expand Down Expand Up @@ -1196,7 +1225,9 @@ void Worker::createMulticastSocketForInterface(const IInterface& iface)
}

auto socket = new QUdpSocket(this);
if(!socket->bind(QHostAddress(), 0, QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint))
qCDebug(netudp_worker_log) << "Create multicast tx socket for iface " << ifaceName;

if(!socket->bind(QHostAddress(QHostAddress::AnyIPv4), 0, QAbstractSocket::ShareAddress | QAbstractSocket::ReuseAddressHint))
{
socket->deleteLater();
return false;
Expand Down
23 changes: 20 additions & 3 deletions tests/Tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace netudp {
class UnicastClientServer : public ::testing::Test
{
protected:
virtual void SetUp()
void init()
{
rx.setRxAddress(serverListeningAddr);
rx.setRxPort(serverListeningPort);
Expand Down Expand Up @@ -70,13 +70,17 @@ class UnicastClientServer : public ::testing::Test

TEST_F(UnicastClientServer, clientToServer)
{
serverListeningPort = 1111;
init();
rx.setUseWorkerThread(false);
tx.setUseWorkerThread(false);
clientToServerTest();
}

TEST_F(UnicastClientServer, clientToServerMultithread)
{
serverListeningPort = 1112;
init();
rx.setUseWorkerThread(true);
tx.setUseWorkerThread(true);
clientToServerTest();
Expand All @@ -86,7 +90,7 @@ TEST_F(UnicastClientServer, clientToServerMultithread)
class MulticastClientServer : public ::testing::Test
{
protected:
virtual void SetUp()
void init()
{
rx.setMulticastGroups({multicastGroup});
rx.setRxPort(multicastPort);
Expand All @@ -97,7 +101,7 @@ class MulticastClientServer : public ::testing::Test
rx.setMulticastLoopback(true);
}

uint16_t multicastPort = 11111;
uint16_t multicastPort = 11112;
QString multicastGroup = QStringLiteral("239.1.2.3");

netudp::Socket tx;
Expand Down Expand Up @@ -149,13 +153,17 @@ class MulticastClientServer : public ::testing::Test

TEST_F(MulticastClientServer, Sync)
{
multicastPort = 1113;
init();
tx.setUseWorkerThread(false);
tx.setUseWorkerThread(false);
serverToClientTest();
}

TEST_F(MulticastClientServer, Async)
{
multicastPort = 1114;
init();
tx.setUseWorkerThread(true);
tx.setUseWorkerThread(true);
serverToClientTest();
Expand Down Expand Up @@ -248,13 +256,15 @@ class MulticastClient2Server : public ::testing::Test

TEST_F(MulticastClient2Server, Sync)
{
multicastPort = 11235;
tx.setUseWorkerThread(false);
tx.setUseWorkerThread(false);
serverToClientTest();
}

TEST_F(MulticastClient2Server, Async)
{
multicastPort = 11236;
tx.setUseWorkerThread(true);
tx.setUseWorkerThread(true);
serverToClientTest();
Expand Down Expand Up @@ -347,15 +357,22 @@ TEST(WorkerMultithreadFuzz, releaseBindedPort)
server.start();

if(spyServerBounded.empty())
{
qDebug() << "Waiting for server bounded";
ASSERT_TRUE(spyServerBounded.wait());
}

if(spyClientBounded.empty())
{
qDebug() << "Waiting for client bounded";
ASSERT_TRUE(spyClientBounded.wait());
}

auto d = server.makeDatagram(10);
std::memset(d->buffer(), 0x12, 10);
server.sendDatagram(d, "127.0.0.1", 1234);

qDebug() << "Waiting for datagram";
ASSERT_TRUE(spyDatagramReceived.wait());
}
}
Expand Down
Loading