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

Fix panic when parsing connection timeout #295

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

sorairolake
Copy link
Contributor

@sorairolake sorairolake commented Dec 12, 2022

Panic if seconds of connection timeout is greater than Duration::MAX or not finite (see Duration::from_secs_f64), so fix this.

Note this can be simplified when Duration::try_from_secs_f64 is stabilized.

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Nice catch!

Two concerns:

  • This solution rejects -0, while HTTPie accepts it. Do we want that?

  • If the timeout is too large then maybe we should set it to Duration::MAX instead? That's still a couple billion years so the user won't be able to tell 🙂

src/cli.rs Outdated Show resolved Hide resolved
Change error message when parsing connection timeout.

Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>
@sorairolake
Copy link
Contributor Author

If the concerns need to be solved, I will do it.

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Looks good. Maybe @ducaale has an opinion about the points from earlier but I don't think they're very important.

@ducaale
Copy link
Owner

ducaale commented Dec 14, 2022

Two concerns:

  • This solution rejects -0, while HTTPie accepts it. Do we want that?
  • If the timeout is too large then maybe we should set it to Duration::MAX instead? That's still a couple billion years so the user won't be able to tell 🙂

It would've been if the first point could be addressed but I don't see it as a blocker 👍

@ducaale ducaale merged commit 4e76508 into ducaale:master Dec 14, 2022
@sorairolake sorairolake deleted the fix/timeout branch December 15, 2022 02:43
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.

3 participants