-
Notifications
You must be signed in to change notification settings - Fork 12
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
martian: improve error logging in response writing #631
Conversation
CloseAfterReply was needed to support KGP in SauceConnect 4.9. It's not needed now.
6449660
to
882c011
Compare
882c011
to
9f13eaa
Compare
9f13eaa
to
4d9c84a
Compare
…n connect In 4b403a3 we added detection of client disconnects. This extends it with later added isClosedConnError() that enables it on Windows.
req := res.Request | ||
|
||
if p.WriteTimeout > 0 { | ||
if deadlineErr := conn.SetWriteDeadline(time.Now().Add(p.WriteTimeout)); deadlineErr != nil { | ||
log.Errorf(req.Context(), "can't set write deadline: %v", deadlineErr) | ||
} | ||
defer conn.SetWriteDeadline(time.Time{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also reset the timer while reading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reading it's ok due to tricky code.
LGTM. It looks like the last commit's message references wrong commit. |
This patch extracts response writing to separate method and uses isClosedConnError() for smarter error logging like we do in case if readRequest().
CloseAfrerReply is removed for easier refactoring and later function reuse.