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

Rework timeout middleware to use http.TimeoutHandler implementation. … #1801

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Mar 4, 2021

Fix #1761 - Reworked timeout middleware to use http.TimeoutHandler implementation.

NB: Replaced ErrorHandler with ErrorMessage and OnTimeoutRouteErrorHandler on config struct (fix #1761) - this is breaking change but for http.TimeoutHandler there is no special case for timeout errors - that handler will take care of it itself.

@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #1801 (736820f) into master (6f9b71c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1801   +/-   ##
=======================================
  Coverage   89.56%   89.57%           
=======================================
  Files          32       32           
  Lines        2646     2648    +2     
=======================================
+ Hits         2370     2372    +2     
  Misses        178      178           
  Partials       98       98           
Impacted Files Coverage Δ
middleware/timeout.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f9b71c...736820f. Read the comment docs.

@aldas aldas force-pushed the fix_1761_timeout_middleware_datarace branch from 6706199 to 0afd577 Compare March 5, 2021 19:49
@aldas aldas force-pushed the fix_1761_timeout_middleware_datarace branch from 0afd577 to 736820f Compare March 5, 2021 19:58
@aldas aldas requested a review from lammel March 5, 2021 20:03
@lammel
Copy link
Contributor

lammel commented Mar 5, 2021

This is quite some interesting code to review @aldas ;-)

As we just introduced the timeout Middleware with v4.2.0 I think a breaking change is acceptable due to a functional issue (datarace). So I'm in favor of merging this for v4.2.1

@lammel lammel added this to the v4.2.1 milestone Mar 5, 2021
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

No datarace found during testing of tests.
Looks good to me.

@lammel lammel merged commit d6127fe into labstack:master Mar 8, 2021
@joewhite86
Copy link

@lammel @aldas why was the ErrorHandler removed?
I want to use a custom error handler, that returns json instead of HTML. How can I achieve this now?

@aldas aldas deleted the fix_1761_timeout_middleware_datarace branch May 23, 2021 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data Race in timeout middleware
3 participants