fix(bindings/bench): Prevent IO from going out of scope #5007
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
goatgoose
changed the title
This reverts commit c84990c.
jmayclin
approved these changes
Jan 8, 2025
maddeleine
approved these changes
Jan 8, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
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.
Description of changes:
#4847 moved the ownership of the IO streams into
TlsConnPair. After this change, the resumption benchmark started failing. I think this change affected theTlsConnPair::splitAPI, which consumed theTlsConnPairand returned the underlying client and serverTlsConnections. However, now that theTlsConnPairalso owns the IO streams, I think this caused the streams to be dropped, which caused the s2n-tls send/recv callbacks to access invalid memory aftersplitwas called.This PR removes the
TlsConnPair::splitAPI to prevent the IO streams from being dropped, and instead provides references to the underlying client and server.The benchmarks were further broken by #4983, which changed the paths to the bench crate. This PR also updates these paths.
Both of these issues were not caught at PR-time since the bench action currently only runs on pushes to main. To catch similar issues in the future, this PR updates the bench action to build and run on PRs (but not publish any metrics until it's merged).
Call-outs:
None
Testing:
The bench action will now run on all PRs, with the authenticate/publish steps skipped. So the bench action on this PR should build and run successfully: https://github.com/aws/s2n-tls/actions/runs/12677767236/job/35333826698?pr=5007
I temporarily allowed the metrics to be published on PRs in c84990c in order to test this step: https://github.com/aws/s2n-tls/actions/runs/12677418611/job/35332715910?pr=5007
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.