services/horizon/internal: Use custom timeout middleware #1870
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tamirms
force-pushed
the
connection-timeout
branch
2 times, most recently
from
5fb2238 to
d032dc6
Compare
tamirms
changed the title
tamirms
force-pushed
the
connection-timeout
branch
from
d032dc6 to
5e8fa3b
Compare
tamirms
changed the title
abuiles
reviewed
Oct 22, 2019
tamirms
force-pushed
the
connection-timeout
branch
from
5e8fa3b to
0c966d9
Compare
bartekn
reviewed
Oct 23, 2019
| cancel() | ||
| if ctx.Err() == context.DeadlineExceeded { | ||
| if mw, ok := w.(middleware.WrapResponseWriter); !ok { | ||
| log.Warn("expected ResponseWriter instance to be WrapResponseWriter") |
There was a problem hiding this comment.
I think this relies on WrapResponseWriter created in loggerMiddleware. If we ever remove this or change the order of middlewares code here will stop working. Can't we just create a new WrapResponseWriter here? It will also remove a need for type checking.
To prevent WriteHeader from being called multiple times we have created a timeout middleware which checks that the response status hasn't already been written before setting the status to http.StatusGatewayTimeout
tamirms
force-pushed
the
connection-timeout
branch
from
0c966d9 to
c1f0ce9
Compare
bartekn
approved these changes
Oct 23, 2019
abuiles
reviewed
Oct 23, 2019
| ctx, cancel := context.WithTimeout(r.Context(), timeout) | ||
| defer func() { | ||
| cancel() | ||
| if ctx.Err() == context.DeadlineExceeded { |
There was a problem hiding this comment.
Do we do any other handling up the hierarchy chain if ct.Err() is different to context.DeadlineExceeded?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
Summary
Close #1684
Instances of "multiple response.WriteHeader calls" appear in horizon logs. The cause of these warnings is that there is a timeout middleware which terminates a request and responds with
http.StatusGatewayTimeoutif the request exceeds some amount of time.This PR fixes this issue by using a custom timeout middleware after the logging middleware. The logging middleware wraps the
http.ResponseWriterin a chiWrapResponseWriterinstance. The custom timeout middleware then checks that the response status has not been set before callingWriteHeader(http.StatusGatewayTimeout)