Update go version & add verification/testing tools #840

coreydaley · 2023-07-19T20:17:51Z

Fixes #

Summary of Changes

PS: Make sure your PR includes/updates tests! If you need help with this part, just ask!

util.go

conn.go

go.mod

codecov · 2023-08-25T18:24:07Z

Codecov Report

❗ No coverage uploaded for pull request base (main@8039329). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 0b06b53 differs from pull request most recent head 97ebd5b. Consider uploading reports for the commit 97ebd5b to get more accurate results

@@           Coverage Diff           @@
##             main     #840   +/-   ##
=======================================
  Coverage        ?   66.47%           
=======================================
  Files           ?       11           
  Lines           ?     1596           
  Branches        ?        0           
=======================================
  Hits            ?     1061           
  Misses          ?      424           
  Partials        ?      111

LICENSE

README.md

prepared_test.go

ghost

Error handling in the package was degraded by changes made in response to lint messages.

ghost · 2023-08-27T15:12:45Z

client.go

@@ -290,7 +294,9 @@ func (d *Dialer) DialContext(ctx context.Context, urlStr string, requestHeader h
 			}
 			err = c.SetDeadline(deadline)
 			if err != nil {
-				c.Close()
+				if err := c.Close(); err != nil {
+					log.Printf("websocket: failed to close network connection: %v", err)


Writing to the application's stderr is unfriendly. If you must handle the error in some way, then return nil, errors.Join(err, c.Close()).

This comment applies to other locations where the package writes to the application's stderr.

ghost · 2023-08-27T15:55:49Z

conn.go

 }

 func hideTempErr(err error) error {
-	if e, ok := err.(net.Error); ok && e.Temporary() {
+	if e, ok := err.(net.Error); ok {


Based the function name, comments at #29 and comments at efd7f76, the purpose of the function is to avoid returning net.Error with Temporary() == true.

In practice, hideTemp error wraps deadline exceeded errors because deadline exceeded is the only temporary error returned from read or write on a connection.

The new code wraps all network errors. Wrapping all errors should be avoided because it can hide useful information from the application.

Either revert back to the original function or remove the function. It's no longer necessary to force Temporary() == false because applications should not call the deprecated Temporary() method.

ghost · 2023-08-27T16:10:42Z

conn.go

@@ -981,7 +1002,9 @@ func (c *Conn) handleProtocolError(message string) error {
 	if len(data) > maxControlFramePayloadSize {
 		data = data[:maxControlFramePayloadSize]
 	}
-	c.WriteControl(CloseMessage, data, time.Now().Add(writeWait))
+	if err := c.WriteControl(CloseMessage, data, time.Now().Add(writeWait)); err != nil {
+		return err


The close message error is secondary to the protocol error. Revert to the original or use errors.Join( errors.New("websocket: " + message), c.WriteControl(CloseMessage, data, time.Now().Add(writeWait))).

ghost · 2023-08-27T16:12:12Z

conn.go

@@ -998,7 +1021,9 @@ func (c *Conn) handleProtocolError(message string) error {
 func (c *Conn) NextReader() (messageType int, r io.Reader, err error) {
 	// Close previous reader, only relevant for decompression.
 	if c.reader != nil {
-		c.reader.Close()
+		if err := c.reader.Close(); err != nil {
+			log.Printf("websocket: discarding reader close error: %v", err)


Return the error instead of writing the application's stderr.

+1 this comment. or take in a logger and use that.

ghost · 2023-08-27T16:19:30Z

conn.go

@@ -1136,7 +1163,9 @@ func (c *Conn) SetCloseHandler(h func(code int, text string) error) {
 	if h == nil {
 		h = func(code int, text string) error {
 			message := FormatCloseMessage(code, "")
-			c.WriteControl(CloseMessage, message, time.Now().Add(writeWait))
+			if err := c.WriteControl(CloseMessage, message, time.Now().Add(writeWait)); err != nil {


Function h is called before returning a close error to the application. The close error is primary to any error writing the close message. Revert h to the original.

ghost · 2023-08-27T16:22:52Z

conn.go

@@ -1161,7 +1190,7 @@ func (c *Conn) SetPingHandler(h func(appData string) error) {
 			err := c.WriteControl(PongMessage, []byte(message), time.Now().Add(writeWait))
 			if err == ErrCloseSent {
 				return nil
-			} else if e, ok := err.(net.Error); ok && e.Temporary() {
+			} else if _, ok := err.(net.Error); ok {


In practice, the original code returned nil for deadline exceeded errors. The new code returns nil for all network errors. Change to detect deadline exceeded errors only.

ghost · 2023-08-27T16:25:51Z

server.go

@@ -183,7 +184,9 @@ func (u *Upgrader) Upgrade(w http.ResponseWriter, r *http.Request, responseHeade
 	}

 	if brw.Reader.Buffered() > 0 {
-		netConn.Close()
+		if err := netConn.Close(); err != nil {
+			log.Printf("websocket: failed to close network connection: %v", err)


Failure to close network connection is secondary to early write by client. Write to application's stderr is unfriendly. Revert to original or use errors.Join.

ghost · 2023-08-27T16:26:33Z

server.go

-	netConn.SetDeadline(time.Time{})
+	if err := netConn.SetDeadline(time.Time{}); err != nil {
+		if err := netConn.Close(); err != nil {
+			log.Printf("websocket: failed to close network connection: %v", err)


Deadline error is primary to close error. Either ignore close error or use errors.Join. This comment applies to other calls to SetDeadline in this file.

ghost · 2023-08-27T16:31:02Z

server.go

+		panic(err)
+	}
+	if err := bw.Flush(); err != nil {
+		log.Printf("websocket: bufioWriterBuffer: Flush: %v", err)


Flush() will not return an error because writeHook.Write always returns a nil error. Change to panic to indicate that no error is expected here.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/gorilla/websocket](https://togithub.com/gorilla/websocket) | require | patch | `v1.5.0` -> `v1.5.1` | --- ### Release Notes <details> <summary>gorilla/websocket (github.com/gorilla/websocket)</summary> ### [`v1.5.1`](https://togithub.com/gorilla/websocket/releases/tag/v1.5.1) [Compare Source](https://togithub.com/gorilla/websocket/compare/v1.5.0...v1.5.1) #### What's Changed - Add check for Sec-WebSocket-Key header by [@hirasawayuki](https://togithub.com/hirasawayuki) in [gorilla/websocket#752 - Changed the method name UnderlyingConn to NetConn by [@JWSong](https://togithub.com/JWSong) in [gorilla/websocket#773 - remove all versions < 1.16 and add 1.18 by [@ChannyClaus](https://togithub.com/ChannyClaus) in [gorilla/websocket#793 - Check for and report bad protocol in TLSClientConfig.NextProtos by [@ChannyClaus](https://togithub.com/ChannyClaus) in [gorilla/websocket#788 - check err before GotConn for trace by [@junnplus](https://togithub.com/junnplus) in [gorilla/websocket#798 - Update README.md by [@coreydaley](https://togithub.com/coreydaley) in [gorilla/websocket#839 - Correct way to save memory using write buffer pool and freeing net.http default buffers by [@FMLS](https://togithub.com/FMLS) in [gorilla/websocket#761 - Update go version & add verification/testing tools by [@coreydaley](https://togithub.com/coreydaley) in [gorilla/websocket#840 - update golang.org/x/net by [@coreydaley](https://togithub.com/coreydaley) in [gorilla/websocket#856 - update GitHub workflows by [@coreydaley](https://togithub.com/coreydaley) in [gorilla/websocket#857 #### New Contributors - [@hirasawayuki](https://togithub.com/hirasawayuki) made their first contribution in [gorilla/websocket#752 - [@JWSong](https://togithub.com/JWSong) made their first contribution in [gorilla/websocket#773 - [@ChannyClaus](https://togithub.com/ChannyClaus) made their first contribution in [gorilla/websocket#793 - [@junnplus](https://togithub.com/junnplus) made their first contribution in [gorilla/websocket#798 - [@coreydaley](https://togithub.com/coreydaley) made their first contribution in [gorilla/websocket#839 - [@FMLS](https://togithub.com/FMLS) made their first contribution in [gorilla/websocket#761 **Full Changelog**: gorilla/websocket@v1.5.0...v1.5.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on Monday" in timezone Europe/Paris, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/cozy/cozy-stack).

rschmied · 2023-11-15T10:10:12Z

Are the comments made by "Ghost" being addressed / considered? From reading it, there's quite a few things that have changed to the worse, I am in particular bitten by #852 Just wondering...

)" This reverts commit 666c197.

This reverts commit 666c197.

paul-leydier · 2024-05-01T14:42:57Z

Are the comments made by "Ghost" being addressed / considered? From reading it, there's quite a few things that have changed to the worse, I am in particular bitten by #852 Just wondering...

I'm in the same situation as you, I had to revert the upgrade to 1.5.1 as it printed millions of logs where none where printed before. I saw that #852 and #878 have been merged, but did not make it to a new version as of right now?

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/gorilla/websocket](https://github.com/gorilla/websocket) | require | patch | `v1.5.0` -> `v1.5.1` | --- ### Release Notes <details> <summary>gorilla/websocket (github.com/gorilla/websocket)</summary> ### [`v1.5.1`](https://github.com/gorilla/websocket/releases/tag/v1.5.1) [Compare Source](gorilla/websocket@v1.5.0...v1.5.1) #### What's Changed - Add check for Sec-WebSocket-Key header by [@hirasawayuki](https://github.com/hirasawayuki) in gorilla/websocket#752 - Changed the method name UnderlyingConn to NetConn by [@JWSong](https://github.com/JWSong) in gorilla/websocket#773 - remove all versions < 1.16 and add 1.18 by [@ChannyClaus](https://github.com/ChannyClaus) in gorilla/websocket#793 - Check for and report bad protocol in TLSClientConfig.NextProtos by [@ChannyClaus](https://github.com/ChannyClaus) in gorilla/websocket#788 - check err before GotConn for trace by [@junnplus](https://github.com/junnplus) in gorilla/websocket#798 - Update README.md by [@coreydaley](https://github.com/coreydaley) in gorilla/websocket#839 - Correct way to save memory using write buffer pool and freeing net.http default buffers by [@FMLS](https://github.com/FMLS) in gorilla/websocket#761 - Update go version & add verification/testing tools by [@coreydaley](https://github.com/coreydaley) in gorilla/websocket#840 - update golang.org/x/net by [@coreydaley](https://github.com/coreydaley) in gorilla/websocket#856 - update GitHub workflows by [@coreydaley](https://github.com/coreydaley) in gorilla/websocket#857 #### New Contributors - [@hirasawayuki](https://github.com/hirasawayuki) made their first contribution in gorilla/websocket#752 - [@JWSong](https://github.com/JWSong) made their first contribution in gorilla/websocket#773 - [@ChannyClaus](https://github.com/ChannyClaus) made their first contribution in gorilla/websocket#793 - [@junnplus](https://github.com/junnplus) made their first contribution in gorilla/websocket#798 - [@coreydaley](https://github.com/coreydaley) made their first contribution in gorilla/websocket#839 - [@FMLS](https://github.com/FMLS) made their first contribution in gorilla/websocket#761 **Full Changelog**: gorilla/websocket@v1.5.0...v1.5.1 </details> --- - [ ] If you want to rebase/retry this MR, check this box ---    See merge request alpine/infra/build-server-status!9

coreydaley force-pushed the coreydaley-patch-1 branch from b785693 to 9a161b0 Compare July 24, 2023 01:10

pull-request-size bot added size/L size/XL and removed size/L labels Jul 24, 2023

Earlopain mentioned this pull request Jul 28, 2023

Discussion: Gorilla websockets are in read-only mode bwmarrin/discordgo#1352

Open

coreydaley changed the title ~~Delete .circleci directory~~ Update go version & add verification/testing tools Jul 30, 2023

This was referenced Jul 30, 2023

Replaced the deprecated ioutil package #776

Closed

Replace deprecated ioutil pkg and bump go.mod to 1.16 #827

Closed

ghost reviewed Aug 14, 2023

View reviewed changes

util.go Outdated Show resolved Hide resolved

conn.go Outdated Show resolved Hide resolved

ghost reviewed Aug 17, 2023

View reviewed changes

go.mod Show resolved Hide resolved

coreydaley force-pushed the coreydaley-patch-1 branch 2 times, most recently from 9b696aa to 9bc6e34 Compare August 25, 2023 18:03

pull-request-size bot added size/XXL and removed size/XL labels Aug 25, 2023

coreydaley force-pushed the coreydaley-patch-1 branch 3 times, most recently from 45ec879 to 0058a29 Compare August 25, 2023 18:22

coreydaley enabled auto-merge (squash) August 25, 2023 18:22

coreydaley requested review from AlexVulaj, apoorvajagtap and a user August 25, 2023 18:29

apoorvajagtap requested changes Aug 25, 2023

View reviewed changes

LICENSE Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

prepared_test.go Outdated Show resolved Hide resolved

coreydaley requested a review from apoorvajagtap August 26, 2023 00:14

Use crypto/rand for mask key

f9cf504

coreydaley force-pushed the coreydaley-patch-1 branch from 0b06b53 to 73bfa98 Compare August 26, 2023 02:33

coreydaley mentioned this pull request Aug 26, 2023

Use crypto/rand for mask key #846

Closed

coreydaley force-pushed the coreydaley-patch-1 branch 3 times, most recently from b761ebe to 7dd2ee2 Compare August 26, 2023 16:09

updating to go1.19 and various other files

97ebd5b

coreydaley force-pushed the coreydaley-patch-1 branch from 7dd2ee2 to 97ebd5b Compare August 26, 2023 18:03

apoorvajagtap approved these changes Aug 26, 2023

View reviewed changes

coreydaley merged commit 666c197 into main Aug 26, 2023
11 checks passed

coreydaley deleted the coreydaley-patch-1 branch August 26, 2023 20:01

ghost reviewed Aug 27, 2023

View reviewed changes

jbarlowbignell mentioned this pull request Sep 19, 2023

[BUG] Debug messages are printed to log #852

Closed

1 task

ghost mentioned this pull request Nov 8, 2023

Do not handle network error in SetCloseHandler() #863

Merged

apoorvajagtap mentioned this pull request Feb 14, 2024

Return errors instead of printing to logs #897

Merged

12 tasks

apoorvajagtap added a commit to apoorvajagtap/gorilla-websocket that referenced this pull request Apr 4, 2024

Revert " Update go version & add verification/testing tools (gorilla#840

b4fae1f

)" This reverts commit 666c197.

apoorvajagtap added a commit to apoorvajagtap/gorilla-websocket that referenced this pull request Apr 4, 2024

Revert " Update go version & add verification/testing tools (gorilla#840

658336d

)" This reverts commit 666c197.

apoorvajagtap added a commit to apoorvajagtap/gorilla-websocket that referenced this pull request Apr 4, 2024

Revert " Update go version & add verification/testing tools (gorilla#840

41b7285

)" This reverts commit 666c197.

apoorvajagtap added a commit that referenced this pull request May 1, 2024

Revert " Update go version & add verification/testing tools (#840)"

b2c246b

This reverts commit 666c197.

gorilla deleted a comment from davidnewhall May 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update go version & add verification/testing tools #840

Update go version & add verification/testing tools #840

coreydaley commented Jul 19, 2023

codecov bot commented Aug 25, 2023 •

edited

ghost left a comment

ghost Aug 27, 2023

ghost Aug 27, 2023

ghost Aug 27, 2023

ghost Aug 27, 2023

davidnewhall Feb 9, 2024

ghost Aug 27, 2023

ghost Aug 27, 2023

ghost Aug 27, 2023

ghost Aug 27, 2023

ghost Aug 27, 2023 •

edited by ghost

rschmied commented Nov 15, 2023

paul-leydier commented May 1, 2024

Update go version & add verification/testing tools #840

Update go version & add verification/testing tools #840

Conversation

coreydaley commented Jul 19, 2023

codecov bot commented Aug 25, 2023 • edited

Codecov Report

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Aug 27, 2023 • edited by ghost

Choose a reason for hiding this comment

rschmied commented Nov 15, 2023

paul-leydier commented May 1, 2024

codecov bot commented Aug 25, 2023 •

edited

ghost Aug 27, 2023 •

edited by ghost