Do not handle network error in `SetCloseHandler()` #863

nak3 · 2023-11-08T12:12:46Z

The 666c197 added an error handling in SetCloseHandler() and peer stops getting CloseError when network issue like write: broken pipe happens because the close handle returns the error.

Hence this patch changes to skip network error handling.

nak3 · 2023-11-08T12:16:04Z

For more detail, we have an unit test as https://github.com/knative/serving/blob/65ce2aece44a5bc5550b54aef657a82f0a6ce61b/pkg/autoscaler/statserver/server_test.go#L111 but it stops getting CloseError on websocket v1.5.1.

coreydaley · 2023-11-13T00:56:46Z

This may have been fixed in #865 can you take a look and verify?

nak3 · 2023-11-13T01:02:49Z

No, #865 does not fix the issue. I looked at and also verified it.

coreydaley · 2023-11-13T01:07:03Z

Then there is a merge conflict with this pull request now that needs to be resolved then.

ghost · 2023-11-13T15:35:32Z

With this change and 865, the close handler will in effect ignore all errors. Consider changing the code to ignore all errors as as the code did prior to August of this year.

It's more important to return the CloseError to the application than any error returned from writing the control message.

nak3 · 2023-11-14T04:45:42Z

With this change and 865, the close handler will in effect ignore all errors.

I think the close handler with this PR will not ignore all errors but will return some error such as errInvalidControlFrame and errWriteTimeout.

Consider changing the code to ignore all errors as as the code did prior to August of this year.
It's more important to return the CloseError to the application than any error returned from writing the control message.

I sort of agree that the close handle should ignore all errors. But I cannot confirm that so this PR ignores network error only for now.

ghost · 2023-11-14T05:08:51Z

errWriteTimeout implements the net.Error interface and is therefore ignored by this PR.
WriteControl(CloseMessage, FormatCloseMessage(code, ""), t) will not return errInvalidControlFrame. We know that CloseMessage is a valid control frame type and that FormatCloseMessage(code, "") is a valid control frame payload.

The websocket connection ignored the error returned from echoing the close message until the PR in August. It seems safe to revert back to the original code.

nak3 · 2023-11-14T05:38:59Z

Okay, thank you. Updated

nak3 · 2023-11-20T02:10:24Z

@coreydaley Could you please take a look?

nak3 · 2023-12-07T02:11:00Z

@coreydaley gentle ping.

AlexVulaj · 2023-12-11T04:26:39Z

There was a recent issue brought up about noisy, unactionable error messages that may be related here: #878

I'm in favor of taking a similar approach and ignoring the error in this specific case. @coreydaley thoughts?

nak3 · 2023-12-12T02:32:06Z

@coreydaley @AlexVulaj Sorry for bothering you. But could you please take a look?

nak3 · 2023-12-16T03:25:46Z

@coreydaley @AlexVulaj gentle ping.

dprotaso · 2024-01-15T21:28:00Z

Hey folks - what's the latest with this PR? Knative is hoping to bump to 1.5.1+ but this is blocking us

The 666c197 added an error handling in `SetCloseHandler()` and peer stops getting `CloseError` when network issue like `write: broken pipe` happens because the close handle returns the error. Hence this patch changes to skip network error handling.

This reverts commit 6db907c.

codecov · 2024-01-22T04:01:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (871f6bb) 70.52% compared to head (42fd2c0) 71.21%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #863      +/-   ##
==========================================
+ Coverage   70.52%   71.21%   +0.69%     
==========================================
  Files          11       11              
  Lines        1591     1584       -7     
==========================================
+ Hits         1122     1128       +6     
+ Misses        358      349       -9     
+ Partials      111      107       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AlexVulaj · 2024-01-22T04:03:15Z

@nak3 @dprotaso hey, sorry about the delay. Change is merged - thanks for contributing!

As gorilla/websocket#863 was merged, we can bump the gorilla websocket to the latest version. Fix knative#14597

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/gorilla/websocket](https://github.com/gorilla/websocket) | require | patch | `v1.5.1` -> `v1.5.3` | --- ### Release Notes <details> <summary>gorilla/websocket (github.com/gorilla/websocket)</summary> ### [`v1.5.3`](https://github.com/gorilla/websocket/releases/tag/v1.5.3) [Compare Source](gorilla/websocket@v1.5.2...v1.5.3) #### Important change This reverts the websockets package back to gorilla/websocket@931041c #### What's Changed - Fixes subprotocol selection (aling with rfc6455) by [@KSDaemon](https://github.com/KSDaemon) in gorilla/websocket#823 - Update README.md, replace master to main by [@mstmdev](https://github.com/mstmdev) in gorilla/websocket#862 - Use status code constant by [@mstmdev](https://github.com/mstmdev) in gorilla/websocket#864 - conn.go: default close handler should not return ErrCloseSent. by [@pnx](https://github.com/pnx) in gorilla/websocket#865 - fix: replace ioutil.readfile with os.readfile by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#868 - fix: add comment for the readBufferSize and writeBufferSize by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#869 - Remove noisy printf in NextReader() and beginMessage() by [@bcreane](https://github.com/bcreane) in gorilla/websocket#878 - docs(echoreadall): fix function echoReadAll comment by [@XdpCs](https://github.com/XdpCs) in gorilla/websocket#881 - make tests parallel by [@ninedraft](https://github.com/ninedraft) in gorilla/websocket#872 - Upgrader.Upgrade: use http.ResposnseController by [@ninedraft](https://github.com/ninedraft) in gorilla/websocket#871 - Do not handle network error in `SetCloseHandler()` by [@nak3](https://github.com/nak3) in gorilla/websocket#863 - perf: reduce timer in write_control by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#879 - fix: lint example code by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#890 - feat: format message type by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#889 - Remove hideTempErr to allow downstream users to check for errors like net.ErrClosed by [@UnAfraid](https://github.com/UnAfraid) in gorilla/websocket#894 - Do not timeout when WriteControl deadline is zero in gorilla/websocket#898 - Excludes errchecks linter by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#904 - Return errors instead of printing to logs by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#897 - Revert " Update go version & add verification/testing tools ([#840](gorilla/websocket#840))" by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#908 - Fixes broken random value generation by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#926 - Reverts back to v1.5.0 by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#929 #### New Contributors - [@KSDaemon](https://github.com/KSDaemon) made their first contribution in gorilla/websocket#823 - [@mstmdev](https://github.com/mstmdev) made their first contribution in gorilla/websocket#862 - [@pnx](https://github.com/pnx) made their first contribution in gorilla/websocket#865 - [@rfyiamcool](https://github.com/rfyiamcool) made their first contribution in gorilla/websocket#868 - [@bcreane](https://github.com/bcreane) made their first contribution in gorilla/websocket#878 - [@XdpCs](https://github.com/XdpCs) made their first contribution in gorilla/websocket#881 - [@ninedraft](https://github.com/ninedraft) made their first contribution in gorilla/websocket#872 - [@nak3](https://github.com/nak3) made their first contribution in gorilla/websocket#863 - [@UnAfraid](https://github.com/UnAfraid) made their first contribution in gorilla/websocket#894 - [@apoorvajagtap](https://github.com/apoorvajagtap) made their first contribution in gorilla/websocket#904 **Full Changelog**: gorilla/websocket@v1.5.1...v1.5.3 ### [`v1.5.2`](https://github.com/gorilla/websocket/releases/tag/v1.5.2) [Compare Source](gorilla/websocket@v1.5.1...v1.5.2) #### What's Changed - Fixes subprotocol selection (aling with rfc6455) by [@KSDaemon](https://github.com/KSDaemon) in gorilla/websocket#823 - Update README.md, replace master to main by [@mstmdev](https://github.com/mstmdev) in gorilla/websocket#862 - Use status code constant by [@mstmdev](https://github.com/mstmdev) in gorilla/websocket#864 - conn.go: default close handler should not return ErrCloseSent. by [@pnx](https://github.com/pnx) in gorilla/websocket#865 - fix: replace ioutil.readfile with os.readfile by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#868 - fix: add comment for the readBufferSize and writeBufferSize by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#869 - Remove noisy printf in NextReader() and beginMessage() by [@bcreane](https://github.com/bcreane) in gorilla/websocket#878 - docs(echoreadall): fix function echoReadAll comment by [@XdpCs](https://github.com/XdpCs) in gorilla/websocket#881 - make tests parallel by [@ninedraft](https://github.com/ninedraft) in gorilla/websocket#872 - Upgrader.Upgrade: use http.ResposnseController by [@ninedraft](https://github.com/ninedraft) in gorilla/websocket#871 - Do not handle network error in `SetCloseHandler()` by [@nak3](https://github.com/nak3) in gorilla/websocket#863 - perf: reduce timer in write_control by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#879 - fix: lint example code by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#890 - feat: format message type by [@rfyiamcool](https://github.com/rfyiamcool) in gorilla/websocket#889 - Remove hideTempErr to allow downstream users to check for errors like net.ErrClosed by [@UnAfraid](https://github.com/UnAfraid) in gorilla/websocket#894 - Do not timeout when WriteControl deadline is zero in gorilla/websocket#898 - Excludes errchecks linter by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#904 - Return errors instead of printing to logs by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#897 - Revert " Update go version & add verification/testing tools ([#840](gorilla/websocket#840))" by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#908 - Fixes broken random value generation by [@apoorvajagtap](https://github.com/apoorvajagtap) in gorilla/websocket#926 #### New Contributors - [@KSDaemon](https://github.com/KSDaemon) made their first contribution in gorilla/websocket#823 - [@mstmdev](https://github.com/mstmdev) made their first contribution in gorilla/websocket#862 - [@pnx](https://github.com/pnx) made their first contribution in gorilla/websocket#865 - [@rfyiamcool](https://github.com/rfyiamcool) made their first contribution in gorilla/websocket#868 - [@bcreane](https://github.com/bcreane) made their first contribution in gorilla/websocket#878 - [@XdpCs](https://github.com/XdpCs) made their first contribution in gorilla/websocket#881 - [@ninedraft](https://github.com/ninedraft) made their first contribution in gorilla/websocket#872 - [@nak3](https://github.com/nak3) made their first contribution in gorilla/websocket#863 - [@UnAfraid](https://github.com/UnAfraid) made their first contribution in gorilla/websocket#894 - [@apoorvajagtap](https://github.com/apoorvajagtap) made their first contribution in gorilla/websocket#904 **Full Changelog**: gorilla/websocket@v1.5.1...v1.5.2 </details> --- - [ ] If you want to rebase/retry this MR, check this box ---    See merge request alpine/infra/build-server-status!14

pull-request-size bot added the size/XS label Nov 8, 2023

nak3 mentioned this pull request Dec 1, 2023

Bump gorilla websocket to 1.5.1+ knative/serving#14597

Closed

ghost mentioned this pull request Dec 11, 2023

[BUG] 1.5.1 uses log.Printf #880

Closed

1 task

FZambia mentioned this pull request Jan 21, 2024

Embed Gorilla WebSocket centrifugal/centrifuge#353

Closed

AlexVulaj approved these changes Jan 22, 2024

View reviewed changes

nak3 added 4 commits January 21, 2024 22:59

Do not return error in SetCloseHandler

8e0711b

Print log message

731d7a3

Revert "Print log message"

42fd2c0

This reverts commit 6db907c.

AlexVulaj merged commit cf50a3e into gorilla:main Jan 22, 2024
11 of 12 checks passed

nak3 added a commit to nak3/serving that referenced this pull request Jan 22, 2024

Bump gorilla websocket to 1.5.1+

9a6ab77

As gorilla/websocket#863 was merged, we can bump the gorilla websocket to the latest version. Fix knative#14597

ReToCode mentioned this pull request Sep 17, 2024

Unpin github.com/gorilla/websocket knative/serving#15515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not handle network error in `SetCloseHandler()` #863

Do not handle network error in `SetCloseHandler()` #863

nak3 commented Nov 8, 2023

nak3 commented Nov 8, 2023

coreydaley commented Nov 13, 2023

nak3 commented Nov 13, 2023

coreydaley commented Nov 13, 2023

ghost commented Nov 13, 2023 •

edited by ghost

Loading

nak3 commented Nov 14, 2023

ghost commented Nov 14, 2023

nak3 commented Nov 14, 2023

nak3 commented Nov 20, 2023

nak3 commented Dec 7, 2023

AlexVulaj commented Dec 11, 2023

nak3 commented Dec 12, 2023

nak3 commented Dec 16, 2023

dprotaso commented Jan 15, 2024

codecov bot commented Jan 22, 2024

AlexVulaj commented Jan 22, 2024

Do not handle network error in SetCloseHandler() #863

Do not handle network error in SetCloseHandler() #863

Conversation

nak3 commented Nov 8, 2023

nak3 commented Nov 8, 2023

coreydaley commented Nov 13, 2023

nak3 commented Nov 13, 2023

coreydaley commented Nov 13, 2023

ghost commented Nov 13, 2023 • edited by ghost Loading

nak3 commented Nov 14, 2023

ghost commented Nov 14, 2023

nak3 commented Nov 14, 2023

nak3 commented Nov 20, 2023

nak3 commented Dec 7, 2023

AlexVulaj commented Dec 11, 2023

nak3 commented Dec 12, 2023

nak3 commented Dec 16, 2023

dprotaso commented Jan 15, 2024

codecov bot commented Jan 22, 2024

Codecov Report

AlexVulaj commented Jan 22, 2024

Do not handle network error in `SetCloseHandler()` #863

Do not handle network error in `SetCloseHandler()` #863

ghost commented Nov 13, 2023 •

edited by ghost

Loading