Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ struct QuicOperatorServerConfig {
* Single operator: `quicServer_.maxConnections = 1` rejects a second concurrent connect attempt at
* the transport layer already (ConnectionShutdown), so — unlike the old hand-rolled version — this
* class no longer needs its own compare-and-swap "already have an operator" logic.
*
* BAF-1900 update: the above does not hold for two handshakes reaching CONNECTED simultaneously —

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL] YouTrack ticket ID in comment: cpp-conventions forbids referencing YouTrack issues/task IDs in comments. This Doxygen doc comment leads with BAF-1900 update:. Describe the reasoning in plain language instead — the ticket ID belongs in the commit/PR, not the code.

* ba-quic-lib's own `QuicServer::onConnected` documents this race as fail-open. `onConnected()`
* does guard `operatorConnection_` with an explicit check-and-set after all, refusing
* (`disconnect()`) any additional operator once one is set (mirrors teleop-module's
* `QuicOperatorServer`).
*/
class QuicOperatorServer {
public:
Expand Down
23 changes: 20 additions & 3 deletions source/transparent_operator_stream/QuicOperatorServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ QuicOperatorServer::QuicOperatorServer(QuicOperatorServerConfig config, Operator
// against quic-lib's QuicServer.cpp) -- so onConnected() below is never called for a connection
// that loses the race, and this class no longer needs its own compare-and-swap "already have an
// operator" logic.
// BAF-1900 update: the above does not hold for two handshakes reaching CONNECTED simultaneously

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL] YouTrack ticket ID in comment: Same issue — the constructor comment leads with BAF-1900 update:. Reword without the ticket ID.

// -- ba-quic-lib's own QuicServer::onConnected documents this race as fail-open. onConnected()
// below therefore does guard operatorConnection_ with an explicit check-and-set after all,
// refusing (disconnect()) any additional operator once one is set (ported from teleop-module's
// QuicOperatorServer, same fix).
quicServer_->maxConnections = 1;
}

Expand All @@ -57,6 +62,9 @@ bringauto::quic::QuicSettings QuicOperatorServer::buildSettings() const {
// (1024) already matches the old PeerUnidiStreamCount setting, so it is left unset here.
settings.idleTimeoutMs = 30000;
settings.keepAliveIntervalMs = 5000;
// Left at ba-quic-lib's own default (5000ms) -- set explicitly so the choice reads as
// deliberate, not inherited (mirrors teleop-module's QuicOperatorServer).
settings.disconnectTimeoutMs = 5000;
settings.sendBufferingEnabled = true;
// 2 == QUIC_SERVER_RESUME_AND_ZERORTT (msquic.h). Spelled out as a literal rather than pulling in
// <msquic.h> for one enum value that ba-quic-lib doesn't itself expose — ba-quic-lib links
Expand Down Expand Up @@ -125,15 +133,24 @@ QuicOperatorServer::SendResult QuicOperatorServer::sendStatus(std::uint32_t modu
}

void QuicOperatorServer::onConnected(ConnectionId id) {
std::lock_guard<std::mutex> lock(operatorMutex_);
operatorConnection_ = std::move(id);
logInfo("QUIC operator server: operator connected");
{
std::lock_guard<std::mutex> lock(operatorMutex_);
if (!operatorConnection_.has_value()) {
operatorConnection_ = std::move(id);
logInfo("QUIC operator server: operator connected");
return;
}
}
logWarning("QUIC operator server: an operator is already connected, rejecting new one "
"(single-operator)");
quicServer_->disconnect(id);
}

void QuicOperatorServer::onDisconnected(ConnectionId id) {
{
std::lock_guard<std::mutex> lock(operatorMutex_);
if (operatorConnection_ != id) {
// BAF-1900: also covers a connection rejected by onConnected()'s own disconnect() call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL] YouTrack ticket ID in comment: Same issue — // BAF-1900: also covers a connection rejected.... Reword without the ticket ID.

return; // a connection rejected by maxConnections never became "the operator"
}
operatorConnection_.reset();
Expand Down