Skip to content
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.

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

Rewrite Python test helpers in Rust #348

Merged
merged 6 commits into from
Oct 6, 2023
Merged

Rewrite Python test helpers in Rust #348

merged 6 commits into from
Oct 6, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Oct 5, 2023

Description

This branch rewrites the verify-static-libraries.py and client-server.py test scripts in Rust. I think this will be helpful longer term because:

  1. There's less mental overhead having to work with just two languages (Rust and C) as opposed to three (Rust, C, and Python)
  2. We don't have to invest in concurrent CI tooling (e.g. for ensuring consistent Python code formatting, linting for common errors, etc).
  3. We don't have to version a second programming language (e.g. managing Python updates over time).

project: bump MSRV 1.60 -> 1.61

This matches a similar update being done upstream in the Rustls crate to accommodate a ring update. It also resolves a build error with the MSRV CI job for the new regex dev-dependency that will be used in the rewritten verify-static-libraries.py test.

tests: RIIR verify-static-libraries.py

Rewrites the verify-static-libraries.py job in Rust. It follows the same overall process:

  1. Running cargo as an external process with RUSTFLAGS set to --print native-static-libs
  2. Regex's the native-static-libs output out of the overall build output
  3. Matches to expected values based on the target OS

We could replace running cargo as a Command in the future by using the cargo crate to compile programmatically, but its API is unstable (and would require more lines of code overall).

The test is marked ignore by default so it won't be run with a standard cargo test invocation. Since this test re-builds the entire crate it's a little bit slow and better suited to be run on-demand like the previous verify-static-libraries.py script was. CI will be updated to run it explicitly in a subsequent commit.

tests: retry connections in client.c

Previously we tried to find out if the server was available by polling the socket in the integration test driver until the port appeared available, and then invoking the client binary.

In practice with the server using SO_REUSEADDR this can result in false positives: the port test in the driver program will appear to connect, but then invoking the client will fail with a connection refused error. I believe this is because the driver is connecting to a lingering socket in the TIME_WAIT state, and when the client is invoked there is a TOCTOU race where the socket has been torn down but the server hasn't started up yet.

Since we don't want to drop the SO_REUSEADDR flag from the server binary (we'd start to see bind failures running the server multiple times due to the lingering sockets from prev. runs) a better solution is to lift the re-connect logic into the client.c program, allowing it up to 10 attempts to connect to the server before declaring defeat. This results in a less flaky experience running the client/server tests in CI.

In practice before making this change I was seeing the Rust-based integration test frequently fail in CI with an unexpected connection refused error. After making this change the jobs have been reliable in my testing.

tests: RIIR client_server tests

This commit implements the client-server.py integration test in Rust.

We ignore this test by default because it requires the tests/client.c and tests/server.c binaries to be built. Those binaries in turn require that the Rust static library is available. Instead we mark the test ignored and can run it explicitly once we've ensured the required bits are in-place. CI will be updated to do so in a follow-up commit.

ci: remove client-server.py, cut over to rust tests

This commit removes the existing client-server.py integration test and updates CI to run the new Rust-based integration tests.

Along the way the Makefile.Windows makefile is updated to correctly list PHONY targets.

tests: drop dedupe from static_libs test

Now that the upstream Cargo nightly bug has been resolved (rust-lang/rust#113209), we don't expect to need to do consecutive deduplication on the native-static-libs output ourselves. This also lets us drop the itertools dev dependency I took to do the de-duping.

cpu added 4 commits October 5, 2023 12:26
This matches a similar update being done upstream in the Rustls crate to
accomodate a *ring* update.
Previously we tried to find out if the server was available by polling
the socket in the integration test driver until the port appeared
available, and then invoking the client binary.

In practice with the server using SO_REUSEADDR this can result in false
positives: the port test in the driver program will appear to connect,
but then invoking the client will fail with a connection refused error.
I believe this is because the driver is connecting to a lingering socket
in the TIME_WAIT state, and when the client is invoked there is a TOCTOU
race where the socket has been torn down but the server hasn't started
up yet.

Since we don't want to drop the SO_REUSEADDR flag from the server
binary a better solution is to lift the re-connect logic into the
`client.c` program, allowing it up to 10 attempts to connect to the
server before declaring defeat. This results in a less flaky experience
running the client/server tests in CI.
This commit implements the `client-server.py` integration test in Rust.

We ignore this test by default because it requires the `tests/client.c`
and `tests/server.c` binaries to be built. Those binaries in turn
require that the Rust static library is available. Instead we mark the
test ignored and can run it explicitly once we've ensured the required
bits are in-place.
Copy link
Collaborator

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for making these changes. One little fix to the ignored flag in the workflow.

cpu added 2 commits October 6, 2023 13:14
This commit removes the existing `client-server.py` integration test and
updates CI to run the new Rust-based integration tests.
Now that the upstream Cargo nightly bug has been resolved, we don't
expect to need to do consecutive deduplication on the native-static-libs
output ourselves.
@jsha
Copy link
Collaborator

jsha commented Oct 6, 2023

As a note, the Build+test (clang, 1.60.0, ubuntu-20.04) status check is in eternally waiting state on this branch, because with changing MSRV that job became Build+test (clang, 1.61.0, ubuntu-20.04). I've removed that as a required status check so we can merge this; we'll add back a required status check for the new version once it lands.

@cpu
Copy link
Member Author

cpu commented Oct 6, 2023

I've removed that as a required status check so we can merge this; we'll add back a required status check for the new version once it lands.

Sounds good. I think it's somewhat useful to have the MSRV version in the task name but I wish it didn't require this dance when we changed it 😵

@cpu cpu merged commit f7df89e into rustls:main Oct 6, 2023
@cpu cpu deleted the cpu-snake-hunt branch October 6, 2023 18:41
@cpu
Copy link
Member Author

cpu commented Oct 6, 2023

we'll add back a required status check for the new version once it lands.

I updated the branch protection rules to re-add all of the current tasks as required with the exception of the optional nightly clippy task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants