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

feat!: Remove until & remove option from accept #41

Merged
merged 1 commit into from Dec 1, 2023
Merged

Conversation

tmccombs
Copy link
Owner

@tmccombs tmccombs commented Dec 1, 2023

Accepting a connection in all cases that I am aware of is an operation that either succeeds or gives an error. It doesn't really make sense for it to return an Option<Result<...>>. Changing the return type to just Result<..> simplifies not only this library, but also any additional implementations of AsyncAccept and any usages of TlsListener.accept().

The one use case, where we needed an Option was if the AsyncAccept trait was configured to end when another future finished first. However, this functionality is already (mostly) available with the StreamExt::take_until method.

I will note that the behavior with take_until is slightly different. With the previous Until impementation, if a tcp connection had been successfully established, but the TLS handshake hadn't been established yet, when the finalization future completed, then we would continue processing that connection.

However, with StreamExt::take_until, such
a connection would be dropped, since it checks the finalizaition future before polling the stream. I hope that in the vast majority of cases this doesn't really matter, or is a better behavior.

BREAKING CHANGE: remove until from AsyncAccept trait. Use
StreamExt.take_until on the TlsListener instead.
BREAKING CHANGE: accept fn on AsyncAccept trait no longer returns an
Option
BREAKING CHANGE: accept fn on TlsListener no longer returns an Option

Accepting a connection in all cases that I am aware of is an operation
that either succeeds or gives an error. It doesn't really make sense for
it to return an `Option<Result<...>>`. Changing the return type to
just `Result<..>` simplifies not only this library, but also any
additional implementations of `AsyncAccept` and any usages of
`TlsListener.accept()`.

The one use case, where we needed an `Option` was if the `AsyncAccept`
trait was configured to end when another future finished first. However,
this functionality is already (mostly) available with the
`StreamExt::take_until` method.

I will note that the behavior with take_until is slightly different.
With the previous `Until` impementation, if a tcp connection had been
successfully established, but the TLS handshake hadn't been established
yet, when the finalization future completed, then we would continue
processing that connection.

However, with `StreamExt::take_until`, such
a connection would be dropped, since it checks the finalizaition future
before polling the stream. I hope that in the vast majority of cases
this doesn't really matter, or is a better behavior.

BREAKING CHANGE: remove `until` from AsyncAccept trait. Use
  `StreamExt.take_until` on the TlsListener instead.
BREAKING CHANGE: `accept` fn on AsyncAccept trait no longer returns an
  Option
BREAKING CHANGE: `accept` fn on TlsListener no longer returns an Option
@tmccombs tmccombs merged commit 9c5d44f into main Dec 1, 2023
13 checks passed
@tmccombs tmccombs deleted the remove-until branch December 1, 2023 07:15
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.

None yet

1 participant