Skip to content

Commit

Permalink
πŸ’₯ Move away from the heresy of mixing unique_ptr with QObject.
Browse files Browse the repository at this point in the history
πŸš‘οΈ Also fix potential deadlock caused by how the memory ownership was managed
  • Loading branch information
OlivierLDff committed Apr 12, 2021
1 parent a335b3b commit d7ba869
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 100 deletions.
4 changes: 2 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ endif()
# OPTIONS

set(NETTCP_VERSION_MAJOR 1)
set(NETTCP_VERSION_MINOR 2)
set(NETTCP_VERSION_PATCH 2)
set(NETTCP_VERSION_MINOR 3)
set(NETTCP_VERSION_PATCH 0)
if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/.git")
execute_process(
COMMAND git rev-parse --short HEAD
Expand Down
25 changes: 17 additions & 8 deletions docs/GettingStart.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ The example is self explanatory.
* Custom signals & slots are present to communicate with `Socket`.

```cpp
#include <Net/Tcp/SocketWorker.hpp>

class MySocketWorker : public net::tcp::SocketWorker
{
Q_OBJECT
Expand Down Expand Up @@ -149,27 +151,30 @@ Now let's implement a custom `Socket`. It will be responsible of:

* Sending string to the worker
* Receiving string from the worker
* Create the wanted custom worker by overriding `std::unique_ptr<net::tcp::SocketWorker> createWorker()`.
* Create the wanted custom worker by overriding `net::tcp::SocketWorker* createWorker()`.

```cpp
#include <Net/Tcp/Socket.hpp>
#include "MySocketWorker.hpp"

class MySocket : public net::tcp::Socket
{
Q_OBJECT
public:
MySocket(QObject* parent = nullptr) : net::tcp::Socket(parent) {}

protected:
std::unique_ptr<net::tcp::SocketWorker> createWorker() override
net::tcp::SocketWorker* createWorker() override
{
auto worker = std::make_unique<MySocketWorker>();
auto worker = new MySocketWorker;

// Send string to worker
connect(this, &MySocket::sendString, worker.get(), &MySocketWorker::onSendString);
connect(this, &MySocket::sendString, worker, &MySocketWorker::onSendString);

// Receive string from worker
connect(worker.get(), &MySocketWorker::stringAvailable, this, &MySocket::stringReceived);
connect(worker, &MySocketWorker::stringAvailable, this, &MySocket::stringReceived);

return std::move(worker);
return worker;
}

Q_SIGNALS:
Expand All @@ -181,6 +186,8 @@ Q_SIGNALS:
### Start the client
```cpp
#include "MySocket.hpp"
int main(int argc, char* argv[])
{
QCoreApplication app(argc, argv);
Expand Down Expand Up @@ -208,11 +215,13 @@ Let's create a `MyServer` that create `MySocket` for each client that connect. W
* Override `bool canAcceptNewClient()` to give condition to accept a client.

```cpp
#include <Net/Tcp/Server.hpp>

class MyServer : public net::tcp::Server
{
Q_OBJECT
protected:
net::tcp::AbstractSocket* newTcpSocket(QObject* parent) override
net::tcp::Socket* newTcpSocket(QObject* parent) override
{
const auto s = new MySocket(parent);
connect(s, &MySocket::stringReceived, [this, s](const QString& string)
Expand Down Expand Up @@ -257,7 +266,7 @@ A watchdog will take care a rebinding to the interface/port if something failed.
It's possible to react to multiple signals from the `Server`.
* `isListeningChanged` tell if the Server is correctly listening to `port` and `address`.
* `void acceptError(int error, const QString description)` indicate an error occured with a client connection.
* `void acceptError(int error, const QString description)` indicate an error occurred with a client connection.
* `void newClient(const QString& address, const quint16 port)` tell when a new client is connected
* `void clientLost(const QString& address, const quint16 port);` tell when a client got disconnected
Expand Down
17 changes: 7 additions & 10 deletions examples/MySocket.cpp
Original file line number Diff line number Diff line change
@@ -1,18 +1,15 @@
#include <MySocket.hpp>
#include <MySocket.hpp>

std::unique_ptr<net::tcp::SocketWorker> MySocket::createWorker()
net::tcp::SocketWorker* MySocket::createWorker()
{
auto worker = std::make_unique<MySocketWorker>();
auto worker = new MySocketWorker;

// Send string to worker
connect(this, &MySocket::sendString, worker.get(),
&MySocketWorker::onSendString);
connect(this, &MySocket::sendErrorString, worker.get(),
&MySocketWorker::onSendErrorString);
connect(this, &MySocket::sendString, worker, &MySocketWorker::onSendString);
connect(this, &MySocket::sendErrorString, worker, &MySocketWorker::onSendErrorString);

// Receive string from worker
connect(worker.get(), &MySocketWorker::stringAvailable, this,
&MySocket::stringReceived);
connect(worker, &MySocketWorker::stringAvailable, this, &MySocket::stringReceived);

return std::move(worker);
return worker;
}
4 changes: 2 additions & 2 deletions examples/MySocket.hpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#ifndef __NETTCP_MYSOCKET_HPP__
#ifndef __NETTCP_MYSOCKET_HPP__
#define __NETTCP_MYSOCKET_HPP__

#include <MySocketWorker.hpp>
Expand All @@ -11,7 +11,7 @@ class MySocket : public net::tcp::Socket
MySocket(QObject* parent = nullptr) : net::tcp::Socket(parent) {};

protected:
std::unique_ptr<net::tcp::SocketWorker> createWorker() override;
net::tcp::SocketWorker* createWorker() override;

Q_SIGNALS:
void sendString(const QString& s);
Expand Down
15 changes: 5 additions & 10 deletions include/Net/Tcp/Server.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@
// Library Headers
#include <Net/Tcp/IServer.hpp>

// Stl Headers
#include <memory>

// ───── DECLARATION ─────

class QTimer;
QT_FORWARD_DECLARE_CLASS(QTimer);

namespace net {
namespace tcp {
Expand Down Expand Up @@ -44,11 +41,9 @@ public Q_SLOTS:
bool start(const QString& address, const quint16 port) override final;
bool stop() override final;
bool restart() override final;
Socket* getSocket(
const QString& address, const quint16 port) const override final;
Socket* getSocket(const QString& address, const quint16 port) const override final;
QList<Socket*> getSockets(const QString& address) const override final;
void disconnectFrom(
const QString& address, const quint16 port) override final;
void disconnectFrom(const QString& address, const quint16 port) override final;
void disconnectFrom(const QString& address) override final;

protected:
Expand All @@ -68,8 +63,8 @@ public Q_SLOTS:
void stopWatchdog();

private:
std::unique_ptr<ServerWorker> _worker;
std::unique_ptr<QTimer> _watchdog;
ServerWorker* _worker = nullptr;
QTimer* _watchdog = nullptr;
};

}
Expand Down
13 changes: 6 additions & 7 deletions include/Net/Tcp/Socket.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@
#include <Net/Tcp/ISocket.hpp>

// Stl Headers
#include <memory>

// ───── DECLARATION ─────

class QThread;
QT_FORWARD_DECLARE_CLASS(QThread);

namespace net {
namespace tcp {
Expand Down Expand Up @@ -50,13 +49,13 @@ public Q_SLOTS:

// ──────── WORKER ────────
private:
std::unique_ptr<SocketWorker> _worker;
std::unique_ptr<QThread> _workerThread;
SocketWorker* _worker = nullptr;
QThread* _workerThread = nullptr;

private Q_SLOTS:
void killWorker();
void onStartSuccess(const QString& peerAddress, const quint16 peerPort,
const QString& localAddress, const quint16 localPort);
void onStartSuccess(
const QString& peerAddress, const quint16 peerPort, const QString& localAddress, const quint16 localPort);
void onStartFail();
void onBytesReceived(const uint64_t count);
void onBytesSent(const uint64_t count);
Expand All @@ -67,7 +66,7 @@ private Q_SLOTS:

// ──────── CUSTOM WORKER API ────────
protected:
virtual std::unique_ptr<class SocketWorker> createWorker();
virtual SocketWorker* createWorker();
};

}
Expand Down
17 changes: 11 additions & 6 deletions src/Server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ using namespace net::tcp;
// ───── CLASS ─────

Server::Server(QObject* parent) :
IServer(parent, {"address", "port", "peerAddress", "peerPort"}), _worker(std::make_unique<ServerWorker>())
IServer(parent, {"address", "port", "peerAddress", "peerPort"}), _worker(new ServerWorker(this))
{
connect(_worker.get(), &ServerWorker::newIncomingConnection, this,
connect(_worker, &ServerWorker::newIncomingConnection, this,
[this](qintptr handle)
{
if(!handle)
Expand Down Expand Up @@ -101,7 +101,7 @@ Server::Server(QObject* parent) :
}
});

connect(_worker.get(), &ServerWorker::acceptError, this,
connect(_worker, &ServerWorker::acceptError, this,
[this](int error)
{
// todo : Use our own enum exposed to qml
Expand Down Expand Up @@ -222,9 +222,9 @@ void Server::startWatchdog()
{
if(!_watchdog)
{
_watchdog = std::make_unique<QTimer>();
_watchdog = new QTimer(this);
connect(
_watchdog.get(), &QTimer::timeout, this,
_watchdog, &QTimer::timeout, this,
[this]()
{
// Watchdog shouldn't be started is worker is listening with success
Expand All @@ -243,7 +243,12 @@ void Server::startWatchdog()
_watchdog->start(watchdogPeriod());
}

void Server::stopWatchdog() { _watchdog = nullptr; }
void Server::stopWatchdog()
{
if(_watchdog)
_watchdog->deleteLater();
_watchdog = nullptr;
}

Socket* Server::newSocket(QObject* parent) { return new Socket(parent); }

Expand Down
Loading

0 comments on commit d7ba869

Please sign in to comment.