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

Remove unused s2n_config_client_hello_cb_enable_poll #3850

Merged
merged 3 commits into from
Feb 24, 2023

Conversation

lrstewart
Copy link
Contributor

Description of changes:

s2n_config_client_hello_cb_enable_poll was initially added to make the ClientHello callback follow the polling model that Rust uses. However, that solution wouldn't have scaled to other async callbacks, and we went with a different solution. s2n_config_client_hello_cb_enable_poll complicates the already complex ClientHello callback logic, so I'd like to remove it.

Here's the commit that added it: 70884dc#diff-4a6a5adbf833174fde04f3043fd5575868bebc5085b7dee3c71abb4270c8d900

It's old enough that I couldn't git revert it without an unreasonable number of conflicts, so I manually reverted it.

Testing:

Existing tests pass. I removed tests that turned on the polling feature, but kept the extra HRR tests.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Feb 24, 2023
@lrstewart lrstewart marked this pull request as ready for review February 24, 2023 16:23
Copy link
Contributor

@harrisonkaiser harrisonkaiser left a comment

Choose a reason for hiding this comment

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

For the future can you comment with a link to what our alternate solution is?

@lrstewart
Copy link
Contributor Author

lrstewart commented Feb 24, 2023

For the future can you comment with a link to what our alternate solution is?

This comment explains it, but the actual implementation is spread over several different commits.

@lrstewart lrstewart enabled auto-merge (squash) February 24, 2023 17:00
@lrstewart lrstewart merged commit 60da828 into aws:main Feb 24, 2023
@lrstewart lrstewart deleted the binding_cleanup branch February 25, 2023 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants