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

Add Middleware.Timeout #702

Merged
merged 2 commits into from
Aug 2, 2018
Merged

Add Middleware.Timeout #702

merged 2 commits into from
Aug 2, 2018

Conversation

pbrisbin
Copy link
Member

See #701.

Wraps the inner respond in System.Timeout.timeout with the given seconds
and, by default, responds status503. The main timeout is implemented in
terms of timeoutStatus for customizing only the response status, which is
itself implemented in terms of timeoutAs, for customizing the Response
entirely.

Some things I'm not sure of:

  1. timeout is a bit of a bold name, but it seems in line with how all
    Middlewares name themselves: assuming a very narrow context
  2. Is timeoutStatus useful, or is just timeout/timeoutAs enough?
  3. If we keep timeoutStatus, should timeoutAs be named timeoutResponse,
    which feels more consistent?
  4. Should we accept microseconds directly? Is there ever a use-case for a
    timeout smaller than one second? (it would be nice to not have to wait an
    entire second for each of the spec examples)

Any other tweaks or style changes, don't hesitate to let me know.

Copy link
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

LGTM! Can you give this a minor version bump in the cabal file, fill in the @since comments in the module for all exported identifiers and the module itself, and add a changelog entry?

@pbrisbin
Copy link
Member Author

Will do!

@pbrisbin
Copy link
Member Author

pbrisbin commented Aug 1, 2018

@snoyberg this should be all set now :)

Copy link
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@snoyberg snoyberg merged commit 0355502 into master Aug 2, 2018
@snoyberg snoyberg deleted the pb/timeout-middleware branch August 2, 2018 05:33
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