Skip to content

Rework timeout implementation to panic when the timer is dropped. #24

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

Closed
wants to merge 1 commit into from

Conversation

rjsberry
Copy link

@rjsberry rjsberry commented Jul 9, 2019

This also changes the extension traits to extend Future/Stream instead of TryFuture/TryStream.

Follows up on review of #22 (sorry about the delay!)

For now I've left this as a draft because I've set the Output/Item in the extension traits to an Option. @yoshuawuyts I'd be interested on your thoughts as to whether we want this to be:

  • Option as is.
  • io::Error as in runtime.
  • Another custom error.
  • Something else.

I don't love Option here, especially with the nested option in the stream extension. I think something like

struct TimeoutError(());

might work quite well instead of io::Error.


Description

This reworks the timer implementation to panic on error instead of returning an io::Error.

Motivation and Context

This simplifies the extension traits and removes the From<io::Error> trait bound from #21.

How Has This Been Tested?

Tests reworked/pass.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@rjsberry
Copy link
Author

@yoshuawuyts Have you had a chance to give this a once over?

This changes the extension traits to extend Future/Stream instead of
TryFuture/TryStream.
@rjsberry
Copy link
Author

I've updated Option to Result using a custom error type over io::Error.

@rjsberry rjsberry marked this pull request as ready for review August 28, 2019 17:49
@yoshuawuyts
Copy link
Contributor

@rjsberry heya! -- sorry about the long wait, I was a bit busy over the past month. Um so there've been a few developments here:

I'm not 100% sure how this relates back to this PR, but I have a hunch it might influence the design taken. I think at least finding a resolution with #26 would probably be good because it seems both PRs have a lot of overlap.

@rjsberry
Copy link
Author

rjsberry commented Sep 3, 2019

No problem!

Yes, this and #26 are quite similar but I think incompatible. This PR promotes extending Future and TryFuture, whereas #26 keeps the extension traits locked to TryFuture.

I'm not sure I agree with the direction of TimeoutError error in #26:

pub enum TimeoutError<E> {
    TimedOut,
    Inner(E),
}

If we parameterize the output of the extension traits with E, are we saying that futures with no error always need to be converted before they can be used with the extension traits? That is exactly what I would like to avoid.

If we agree the timer implementation should panic on error, I think the extension trait will work best as

pub trait FutureExt: Future {
    fn timeout(self, dur: Duration) -> Timeout { ... }
}

Instead of

pub trait TryFutureExt: TryFuture {
    fn timeout(self, dur: Duration) -> Timeout { ... }
}

This was referenced Oct 9, 2019
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