Skip to content

qt: make the WebSocket backend and server clang-tidy clean (#514) #516

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.

Sign up for GitHub

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

Merged
Yaraslaut merged 1 commit into from
Sep 11, 2026
Merged

qt: make the WebSocket backend and server clang-tidy clean (#514) #516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion include/morph/qt/qt_websocket_backend.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,14 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend {
/// @brief Closes the socket and cleans up pending operations.
~QtWebSocketBackend() override;

// Neither copyable nor movable: the backend owns a QWebSocket bound to this
// object's address through Qt's signal/slot connections, and its pending-call
// maps are keyed to callbacks that capture `this`.
QtWebSocketBackend(const QtWebSocketBackend&) = delete;
QtWebSocketBackend& operator=(const QtWebSocketBackend&) = delete;
QtWebSocketBackend(QtWebSocketBackend&&) = delete;
QtWebSocketBackend& operator=(QtWebSocketBackend&&) = delete;

/// @brief Pumps the Qt event loop until the socket is connected or @p timeoutMs elapses.
///
/// Must be called on the Qt event loop thread after construction.
Expand Down Expand Up @@ -498,7 +506,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend {
struct PendingExecute {
std::shared_ptr<::morph::async::detail::CompletionState<std::shared_ptr<void>>> state;
std::function<std::shared_ptr<void>(std::string_view)> deserialize;
::morph::exec::IExecutor* cbExec;
::morph::exec::IExecutor* cbExec{nullptr};
};
uint64_t _nextCallId{0};
std::unordered_map<uint64_t, PendingExecute> _pending;
Expand Down
13 changes: 10 additions & 3 deletions include/morph/qt/qt_websocket_server.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@ class QtWebSocketServer : public QObject {
/// @brief Closes the server and disconnects all clients.
~QtWebSocketServer() override;

// Neither copyable nor movable: a QObject with signal/slot connections bound
// to this address, holding per-client state keyed by QWebSocket pointer.
QtWebSocketServer(const QtWebSocketServer&) = delete;
QtWebSocketServer& operator=(const QtWebSocketServer&) = delete;
QtWebSocketServer(QtWebSocketServer&&) = delete;
QtWebSocketServer& operator=(QtWebSocketServer&&) = delete;

/// @brief Starts listening for incoming WebSocket connections.
///
/// Refuses — returns `false` without binding, and logs at
Expand Down Expand Up @@ -202,10 +209,10 @@ class QtWebSocketServer : public QObject {
double tokens = 0.0;

/// @brief Last time `tokens` was refilled (used to compute elapsed time on the next frame).
std::chrono::steady_clock::time_point lastRefill{};
std::chrono::steady_clock::time_point lastRefill;

/// @brief Last time any frame was received on this connection (drives `idleTimeout`).
std::chrono::steady_clock::time_point lastActivity{};
std::chrono::steady_clock::time_point lastActivity;

/// @brief One-shot timer enforcing `handshakeTimeout`; `nullptr` once cancelled by the
/// first frame, or if `handshakeTimeout == 0`.
Expand All @@ -222,7 +229,7 @@ class QtWebSocketServer : public QObject {
/// @param state Per-connection state to update.
/// @return `true` if a token was available (the frame is admitted), `false` if
/// the bucket was empty (the frame must be dropped).
bool consumeToken(ClientState& state);
bool consumeToken(ClientState& state) const;

/// @brief Qt slot: periodic sweep that closes any connection idle past `idleTimeout`.
Q_SLOT void onHousekeepingTick();
Expand Down
51 changes: 34 additions & 17 deletions src/qt/qt_websocket_backend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,10 @@ bool QtWebSocketBackend::registerModelAsync(
// happens, cancelPending() drains this queue too and still invokes
// onError exactly once.
std::scoped_lock const lock{_pendingMtx};
_queuedRegistrations.push_back(
QueuedRegistration{typeId, std::string{contextKey}, std::move(onRegistered), std::move(onError)});
_queuedRegistrations.push_back(QueuedRegistration{.typeId = typeId,
.contextKey = std::string{contextKey},
.onRegistered = std::move(onRegistered),
.onError = std::move(onError)});
return true;
}
sendRegisterAsync(typeId, contextKey, std::move(onRegistered), std::move(onError));
Expand All @@ -204,7 +206,8 @@ void QtWebSocketBackend::sendRegisterAsync(const std::string& typeId, std::strin
auto const encoded = QString::fromStdString(::morph::wire::encode(env));
{
std::scoped_lock const lock{_pendingMtx};
_pendingRegistrations[callId] = PendingRegistration{std::move(onRegistered), std::move(onError)};
_pendingRegistrations[callId] =
PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)};
}
_socket.sendTextMessage(encoded);
}
Expand Down Expand Up @@ -391,7 +394,8 @@ bool QtWebSocketBackend::assignPrimaryAsync(::morph::exec::detail::ModelId mid,
auto const encoded = QString::fromStdString(::morph::wire::encode(env));
{
std::scoped_lock const lock{_pendingMtx};
_pendingAssigns[callId] = PendingAssign{std::move(onRegistered), std::move(onError)};
_pendingAssigns[callId] =
PendingAssign{.onRegistered = std::move(onRegistered), .onError = std::move(onError)};
}
_socket.sendTextMessage(encoded);
return true;
Expand Down Expand Up @@ -446,7 +450,7 @@ ::morph::async::Completion<std::shared_ptr<void>> QtWebSocketBackend::execute(
return comp;
}

uint64_t callId = ++_nextCallId;
uint64_t const callId = ++_nextCallId;
::morph::wire::Envelope env;
env.kind = "execute";
env.callId = callId;
Expand All @@ -457,8 +461,9 @@ ::morph::async::Completion<std::shared_ptr<void>> QtWebSocketBackend::execute(
env.session = std::move(call.session);

{
std::scoped_lock lock{_pendingMtx};
_pending[callId] = PendingExecute{compState, std::move(call.deserializeResult), cbExec};
std::scoped_lock const lock{_pendingMtx};
_pending[callId] =
PendingExecute{.state = compState, .deserialize = std::move(call.deserializeResult), .cbExec = cbExec};
}

_socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env)));
Expand All @@ -471,7 +476,7 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) {
std::vector<QueuedRegistration> drainedQueue;
std::unordered_map<uint64_t, PendingAssign> drainedAssigns;
{
std::scoped_lock lock{_pendingMtx};
std::scoped_lock const lock{_pendingMtx};
drainedExecutes.swap(_pending);
drainedRegistrations.swap(_pendingRegistrations);
drainedQueue.swap(_queuedRegistrations);
Expand All @@ -480,20 +485,27 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) {
// just drop the bookkeeping, there is no callback to invoke.
_pendingDeregisters.clear();
}
for (auto& [_, pending] : drainedExecutes) {
for (auto& [ignoredCallId, pending] : drainedExecutes) {
if (pending.state) {
pending.state->setException(exc);
}
}
std::string message = "disconnected";
// Every caller passes a `make_exception_ptr`, so `rethrow_exception` always
// throws and one of the two handlers always assigns `message`.
std::string message;
try {
std::rethrow_exception(exc);
} catch (const std::exception& concrete) {
message = concrete.what();
} catch (...) {
// Non-std::exception thrown in: keep the "disconnected" fallback above.
}
for (auto& [_, pending] : drainedRegistrations) {
// A non-std::exception carries no portable message, so report the
// disconnect itself -- which is what a caller can act on anyway. Doing
// it here rather than as an initializer above keeps the handler from
// being lexically empty, which `bugprone-empty-catch` rejects however
// well the intent is commented (morph#514).
message = "disconnected";
}
for (auto& [ignoredCallId, pending] : drainedRegistrations) {
if (pending.onError) {
pending.onError(message);
}
Expand All @@ -509,7 +521,7 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) {
entry.onError(message);
}
}
for (auto& [_, pending] : drainedAssigns) {
for (auto& [ignoredCallId, pending] : drainedAssigns) {
if (pending.onError) {
pending.onError(message);
}
Expand All @@ -527,8 +539,13 @@ void QtWebSocketBackend::setSession(::morph::session::Context session) { _sessio
void QtWebSocketBackend::scheduleReconnect() {
_reconnectTimer.start(static_cast<int>(_currentReconnectDelay.count()));
// Pre-compute the next backoff so the timer above used the *current* one.
auto next = std::chrono::milliseconds{
static_cast<std::chrono::milliseconds::rep>(_currentReconnectDelay.count() * _cfg.backoffMultiplier)};
// Cast up to double first so the multiplication is openly floating-point.
// Written as `count() * backoffMultiplier` the integral `rep` is narrowed to
// double *inside* the expression, which the narrowing-conversions checks
// flag separately from the explicit cast back (morph#514). Same arithmetic,
// same result -- only the one deliberate narrowing is left, on the outside.
auto next = std::chrono::milliseconds{static_cast<std::chrono::milliseconds::rep>(
static_cast<double>(_currentReconnectDelay.count()) * _cfg.backoffMultiplier)};
_currentReconnectDelay = std::min(next, _cfg.maxReconnectDelay);
}

Expand Down Expand Up @@ -694,7 +711,7 @@ void QtWebSocketBackend::onTextMessage(const QString& message) {
}

_pendingReply = std::move(msg);
if (_syncLoop) {
if (_syncLoop != nullptr) {
_syncLoop->quit();
}
}
Expand Down
29 changes: 18 additions & 11 deletions src/qt/qt_websocket_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ QtWebSocketServer::QtWebSocketServer(::morph::backend::RemoteServer& server, qui
: QObject{parent},
_server{server},
_requestedPort{port},
_cfg{cfg},
_cfg{std::move(cfg)},
#ifndef QT_NO_SSL
_wsServer{QStringLiteral("morph"),
tls.has_value() ? QWebSocketServer::SecureMode : QWebSocketServer::NonSecureMode, this},
Expand Down Expand Up @@ -73,7 +73,7 @@ void QtWebSocketServer::close() {
for (auto& [socket, state] : _clients) {
socket->disconnect(this);
_server.closeConnection(state.cid);
if (state.handshakeTimer) {
if (state.handshakeTimer != nullptr) {
state.handshakeTimer->stop();
state.handshakeTimer->deleteLater();
}
Expand Down Expand Up @@ -149,7 +149,7 @@ bool QtWebSocketServer::closeGracefully(std::chrono::milliseconds deadline) {

void QtWebSocketServer::onNewConnection() {
QWebSocket* socket = _wsServer.nextPendingConnection();
if (!socket) {
if (socket == nullptr) {
return;
}
if (_cfg.maxConnections != 0 && _clients.size() >= _cfg.maxConnections) {
Expand All @@ -176,6 +176,13 @@ void QtWebSocketServer::onNewConnection() {
::morph::log::logInfo("[QtWebSocketServer] connection {} accepted ({} live)", state.cid, _clients.size() + 1);

if (_cfg.handshakeTimeout.count() > 0) {
// Qt parent-child ownership: `this` owns the timer and deletes it, and every path that
// drops the reference calls deleteLater() first (see the handshakeTimer handling below).
// cppcoreguidelines-owning-memory has no model of that convention -- it flags any raw
// `new` bound to a non-gsl::owner pointer (morph#514). Suppressed here rather than for the
// whole directory: this is the only such site in src/, and a directory-wide disable would
// turn the check off for a future `new` that really is unowned.
// NOLINTNEXTLINE(cppcoreguidelines-owning-memory) — QObject parent owns this
auto* timer = new QTimer(this);
timer->setSingleShot(true);
connect(timer, &QTimer::timeout, this, [this, socket] {
Expand All @@ -190,15 +197,15 @@ void QtWebSocketServer::onNewConnection() {
_clients.emplace(socket, state);
}

bool QtWebSocketServer::consumeToken(ClientState& state) {
bool QtWebSocketServer::consumeToken(ClientState& state) const {
if (_cfg.messagesPerSecond == 0) {
return true; // unbounded (today's behavior)
}
auto const now = std::chrono::steady_clock::now();
double const elapsedSeconds = std::chrono::duration<double>(now - state.lastRefill).count();
state.lastRefill = now;
double const capacity = static_cast<double>(_cfg.messagesPerSecond);
state.tokens = std::min(capacity, state.tokens + elapsedSeconds * capacity);
auto const capacity = static_cast<double>(_cfg.messagesPerSecond);
state.tokens = std::min(capacity, state.tokens + (elapsedSeconds * capacity));
if (state.tokens < 1.0) {
return false;
}
Expand All @@ -208,7 +215,7 @@ bool QtWebSocketServer::consumeToken(ClientState& state) {

void QtWebSocketServer::onTextMessage(const QString& message) {
auto* socket = qobject_cast<QWebSocket*>(sender());
if (!socket) {
if (socket == nullptr) {
return;
}
auto iter = _clients.find(socket);
Expand All @@ -217,7 +224,7 @@ void QtWebSocketServer::onTextMessage(const QString& message) {
}
ClientState& state = iter->second;
state.lastActivity = std::chrono::steady_clock::now();
if (state.handshakeTimer) {
if (state.handshakeTimer != nullptr) {
state.handshakeTimer->stop();
state.handshakeTimer->deleteLater();
state.handshakeTimer = nullptr;
Expand Down Expand Up @@ -259,7 +266,7 @@ void QtWebSocketServer::onTextMessage(const QString& message) {
return;
}

QPointer<QWebSocket> weakSocket{socket};
QPointer<QWebSocket> const weakSocket{socket};
_server.handle(
message.toStdString(),
[weakSocket](const std::string& reply) {
Expand All @@ -277,7 +284,7 @@ void QtWebSocketServer::onTextMessage(const QString& message) {

void QtWebSocketServer::onDisconnected() {
auto* socket = qobject_cast<QWebSocket*>(sender());
if (!socket) {
if (socket == nullptr) {
return;
}
auto iter = _clients.find(socket);
Expand All @@ -290,7 +297,7 @@ void QtWebSocketServer::onDisconnected() {
iter->second.cid, _clients.size() - 1, static_cast<int>(socket->closeCode()),
socket->closeReason().toStdString());
_server.closeConnection(iter->second.cid);
if (iter->second.handshakeTimer) {
if (iter->second.handshakeTimer != nullptr) {
iter->second.handshakeTimer->stop();
iter->second.handshakeTimer->deleteLater();
}
Expand Down
Loading