Remove all panics from TCP RR code - #1928
Conversation
This comment has been minimized.
This comment has been minimized.
![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: 26351d03cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| wait_for_shutdown_or_failure(shutdown, &mut fail_rx).await; | ||
| shutdown_flag.store(true, Relaxed); |
There was a problem hiding this comment.
When a server worker returns an I/O error after startup, this new failure wait shuts down every TCP RR server thread and run_server returns the error, but inner_main only logs blackhole errors at lading/src/bin/lading.rs:568-572 and does not monitor them in a join set. The generator and experiment therefore continue for the configured duration after the entire blackhole has disappeared, producing incomplete or misleading load measurements; the blackhole task must propagate the failure to the experiment controller or trigger global shutdown.
AGENTS.md reference: AGENTS.md:L74-L78
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Ugh. I was hoping we were handling that properly already but it looks like the comment is right. We only log a warning for failures in black holes and target metrics. That's rather unfortunate.
It does look like main does the right thing for observers, generators & target errors.
This feels both slightly in scope and out of scope of your PR.
If you feel comfortable enough to change lading's main to fix this, that would be great. If you want to punt that over to me/team, that's understandable.
Let me know, we'll likely want to get that fix in before this PR gets merged.
There was a problem hiding this comment.
(I don't have any other comments for this PR, everything else looks good to me)
There was a problem hiding this comment.
Hey I can take a look at it. Maybe it is better to address that as a separate PR
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ed7cb5877
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let socket = socket2::Socket::new(domain, socket2::Type::STREAM, Some(socket2::Protocol::TCP))?; | ||
| socket.set_nonblocking(true)?; | ||
| socket.set_cloexec(true)?; | ||
| socket.set_reuse_address(true)?; |
There was a problem hiding this comment.
When any of these socket setup operations fails, each ? converts the error through transparent Error::Io, so users receive only a bare OS message and cannot determine whether socket creation, nonblocking mode, close-on-exec, or SO_REUSEADDR failed. Add contextual thiserror variants (including the address and failed operation) rather than replacing the previous diagnostic with an ambiguous error.
AGENTS.md reference: AGENTS.md:L74-L78
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e4d89396f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Err(e) = wait_for_generator( | ||
| &control_listener, | ||
| control_addr, | ||
| params.flows, |
There was a problem hiding this comment.
When a ready server worker fails before the control handshake completes—for example, an early data-port connection encounters a registration or reregistration error—the worker sends fail_tx, but this path awaits only wait_for_generator; fail_rx is not polled until after the flow count has been sent. The control port can therefore continue advertising a blackhole whose data thread has already exited, potentially causing the generator to open fewer or no usable flows; select on the failure receiver during this startup wait and shut down immediately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That is not an issue. Data thread failing will abort the run anyway.
* refactor rr * do not swallow poll registry errors * coordinate number of flows between generator and blackhole * remove non ascii chars from comments * fmt * ensure shutdown flag is set on error paths * decompose run_server * update h2 due to security advisory * ecapsulate control handshake * Remove all panics from TCP RR code (#1928) * bubble up errors * move ready signal right before the event loop * fail fast instead of waiting for shutdown signal after experiment duration * error when shutdown happens before blackhole or generator ready * encapsulate control handshake * enrich socket errors with more context * enrich poll errors with more context
2 participants
What does this PR do?
Remove all panics and replace them with proper error handling.
Motivation
Related issues
Additional Notes