Give remote-test-client a configurable timeout #126959
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
A-testsuite
| match client.read_exact(&mut b) { | ||
| Ok(_) => break, | ||
| Err(e) if e.kind() == io::ErrorKind::WouldBlock => panic!( | ||
| "TCP timeout of {timeout}ms exceeded, set `{TCP_TIMEOUT_ENV}` to a larger value" |
There was a problem hiding this comment.
I'd have expected https://doc.rust-lang.org/nightly/std/io/enum.ErrorKind.html#variant.TimedOut, not WouldBlock?
There was a problem hiding this comment.
That would be a more specific and appropriate error, yes! I wonder why I saw what I did while testing...
|
I think I'd expect us to just increase the timeout rather than making it configurable. I'm not sure there's any significant value of 100ms vs. something like 2 seconds (which seems likely to be enough for your use case?) |
|
I'm perfectly happy to simply increase the timeout to 2s! Let me do that instead! |
|
Closing in favor of #127246 |
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 3, 2024
…ient-has-longer-timeout, r=Mark-Simulacrum Give remote-test-client a longer timeout In rust-lang#126959, ``@Mark-Simulacrum`` suggested we simply extend the timeout of the `remote-test-client` instead of making it configurable. This is acceptable for my use case. I'm doing some work with a remote host running tests using `x.py`'s remote test runner system. After building the `remote-test-server` with: ```bash ./x build src/tools/remote-test-server --target aarch64-unknown-linux-gnu ``` I can transfer the `remote-test-server` to the remote and set up a port forwarded SSH connection, then I run: ```bash TEST_DEVICE_ADDR=127.0.0.1:12345 ./x.py test library/core --target aarch64-unknown-linux-gnu ``` This works great if the host is nearby, however if the average round trip time is over 100ms (the timeout), it creates problems as it silently trips the timeout. This PR extends that timeout to a less strict 2s. r? ``@Mark-Simulacrum``
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 3, 2024
Rollup merge of rust-lang#127246 - ferrocene:hoverbear/remote-test-client-has-longer-timeout, r=Mark-Simulacrum Give remote-test-client a longer timeout In rust-lang#126959, ``@Mark-Simulacrum`` suggested we simply extend the timeout of the `remote-test-client` instead of making it configurable. This is acceptable for my use case. I'm doing some work with a remote host running tests using `x.py`'s remote test runner system. After building the `remote-test-server` with: ```bash ./x build src/tools/remote-test-server --target aarch64-unknown-linux-gnu ``` I can transfer the `remote-test-server` to the remote and set up a port forwarded SSH connection, then I run: ```bash TEST_DEVICE_ADDR=127.0.0.1:12345 ./x.py test library/core --target aarch64-unknown-linux-gnu ``` This works great if the host is nearby, however if the average round trip time is over 100ms (the timeout), it creates problems as it silently trips the timeout. This PR extends that timeout to a less strict 2s. r? ``@Mark-Simulacrum``
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm doing some work with a remote host running tests using
x.py's remote test runner system.After building the
remote-test-serverwith:I can transfer the
remote-test-serverto the remote and set up a port forwarded SSH connection, then I run:TEST_DEVICE_ADDR=127.0.0.1:12345 ./x.py test library/core --target aarch64-unknown-linux-gnuThis works great if the host is nearby, however if the average round trip time is over 100ms (the timeout), it creates problems as it silently trips the timeout.
This PR checks an environment variable for a user configured timeout, defaulting to the old setting of 100 if it's not present. I picked the environment name because it seemed obvious, but there may be another option more suitable!
Also included is an explicit panic with a (hopefully) useful message if the deadline does get tripped. If it's more appropriate, I could make this a non-panic and simply a printed warning.
Testing
To test, you could run the
remote-test-serversimilar to as described in https://rustc-dev-guide.rust-lang.org/tests/running.html#running-tests-on-a-remote-machine.Then, in another terminal:
TCP_TIMEOUT=300 TEST_DEVICE_ADDR=127.0.0.1:12345 ./x.py test tests/uiTo trigger the timeout, you'll need to introduce latency into your loopback and tune the timeout down to something below that.
That message looks like this: