Repository navigation
Initial libquicr session implementation - #189
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ab5fdaa28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| connection_->SetDelegate(shared_from_this()); | ||
|
|
||
| SPDLOG_LOGGER_DEBUG(LOGGER, "Control stream ID {0}", control_data_ctx_id_); | ||
| // The peer answers on the same stream, so the control exchange needs a bidirectional one | ||
| control_stream_ = transport_->CreateRequestStream(connection_); |
There was a problem hiding this comment.
When an outbound connection reaches kReady before SetDelegate() is called, attaching the delegate replays that status and OnConnectionStatus() immediately calls SendConnect(). Because control_stream_ is not created until afterward, SendControl() drops the only CONNECT message and the peering handshake remains stuck. Create the request stream before attaching the delegate, or retry the handshake after the stream exists.
Useful? React with 👍 / 👎.
| peer_sess = std::move(peer_it->second); | ||
| server_peer_sessions_.erase(peer_it); |
There was a problem hiding this comment.
When OnConnectionClosed is delivered before the delegate's disconnected status, this erases the inbound session and then calls SessionChanged(). That cleanup calls SubscribeInfoReceived(peer_session_id, ..., true), whose first operation is GetPeerSession(peer_session_id); the lookup now throws and its catch skips the subscription withdrawal, leaving stale subscriber state and forwarding decisions. Run SessionChanged() before erasing the session, or make disconnect cleanup independent of that lookup.
Useful? React with 👍 / 👎.
| session_manager_->AddTransport(server_config_, | ||
| std::static_pointer_cast<quicr::Session::ServerCallbacks>(shared_from_this())); | ||
|
|
||
| return quicr::Session::Status::kReady; |
There was a problem hiding this comment.
When the server transport cannot be added or become ready—for example because the listening port is already occupied or TLS configuration is invalid—Start() still returns kReady without checking the result or transport status. main.cc relies on this return value to detect startup failure, so the process can remain running even though it is not accepting clients. Return the actual listener/session status or propagate an AddTransport failure.
Useful? React with 👍 / 👎.
| // Peer address requires the owning transport; SessionManager does not expose it. See libquicr issue. | ||
| const auto [ip, port] = GetPeerAddress(nullptr, session->GetConnection()); | ||
| metrics_publisher_.AddConnection(connection_handle, { ip, port }); |
There was a problem hiding this comment.
For every accepted client this passes a literal null transport to GetPeerAddress(), whose first branch always returns an empty address and port zero. Consequently all connection metric samples now lose remote_ip and remote_port, even though those fields previously came from NewConnectionAccepted and remain part of the documented metrics payload. Obtain the owning transport through the session manager/callback rather than invoking the helper with nullptr.
Useful? React with 👍 / 👎.
2 participants
No description provided.