Repository: gorilla/websocket
Pull request #894 (2024-02-07 23:39:50)

Remove hideTempErr to allow downstream users to check for errors like net.ErrClosed
## What type of PR is this? (check all applicable)

- [x] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
- [ ] Go Version Update
- [ ] Dependency Update

## Description
Since this change https://github.com/gorilla/websocket/pull/840/files#diff-4f427d2b022907c552328e63f137561f6de92396d7a6e8f6c2ea1bcf0db52654L190-R197 we can no longer determinate if the errors coming from ReadMessage() are net.ErrClosed for example
Hardcoding the error message is not great option because it may vary from OS to OS and system locale

## Related Tickets & Documents
- Related Issue # https://github.com/gorilla/websocket/issues/877
- Closes # https://github.com/gorilla/websocket/issues/877

## Added/updated tests?

- [X] Yes
- [ ] No, and this is why: _please replace this line with details on why tests
      have not been included_
- [ ] I need help with writing tests

## Run verifications and test

- [X] `make verify` is passing
- [X] `make test` is passing


GreenMarmot: Thank you! The maintainer's decision to hide all errors from the application was mystifying and is one of the reasons we have not upgraded past 1.5.0.  
StevenACoffman: @AlexVulaj This would fix several issues with the 1.5.1 release. Please take a look. Thanks!
GreenMarmot: Prior to commit 666c197fc9157896b57515c3a3326c3f8c8319fe, hideTempError wrapped errors with e.Temporary() == true.  All other errors were passed through as is.  The PR that added hideTempError says "Do not return net.Error with Temporary() == true".   This was probably done because errors on the connection are not recoverable.
 
The net.Error Temporary() method in the standard library was recently deprecated because the meaning of the method was not well-defined. 

This PR brings the package closer to 1.50 by not wrapping any errors.

Regarding the parallel test:  #872 sprinkled t.Parallel() through almost all tests, including [tests where it makes no sense](https://github.com/gorilla/websocket/blob/d08ee1ad9b8a1bad1b388165f85f7794a604b8e1/example_test.go#L45).  For better or worse, the call to t.Parallel() goes with the flow.  
AlexVulaj: I'm happy to merge this @jaitaiwan , feel free to make a final call on the "t.Parallel" comment but I don't have any strong feelings.
UnAfraid: Please let me know if t.Parallel() should be used and i can bring it back, i don't mind either way.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/gorilla/websocket/pull/894?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`d293aa5`)](https://app.codecov.io/gh/gorilla/websocket/commit/d293aa53e1c734d8f2b75c1992134d0b99d21917?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla) 71.30% compared to head [(`52188df`)](https://app.codecov.io/gh/gorilla/websocket/pull/894?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla) 71.38%.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #894      +/-   ##
==========================================
+ Coverage   71.30%   71.38%   +0.08%     
==========================================
  Files          11       11              
  Lines        1589     1583       -6     
==========================================
- Hits         1133     1130       -3     
+ Misses        349      347       -2     
+ Partials      107      106       -1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/gorilla/websocket/pull/894?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla).

StevenACoffman: Test and lint failures are unrelated to this change.

Patch:
diff --git a/conn.go b/conn.go
index 2ac19e9..72f5df8 100644
--- a/conn.go
+++ b/conn.go
@@ -192,13 +192,6 @@ func newMaskKey() [4]byte {
 	return k
 }
 
-func hideTempErr(err error) error {
-	if e, ok := err.(net.Error); ok {
-		err = &netError{msg: e.Error(), timeout: e.Timeout()}
-	}
-	return err
-}
-
 func isControl(frameType int) bool {
 	return frameType == CloseMessage || frameType == PingMessage || frameType == PongMessage
 }
@@ -364,7 +357,6 @@ func (c *Conn) RemoteAddr() net.Addr {
 // Write methods
 
 func (c *Conn) writeFatal(err error) error {
-	err = hideTempErr(err)
 	c.writeErrMu.Lock()
 	if c.writeErr == nil {
 		c.writeErr = err
@@ -1033,7 +1025,7 @@ func (c *Conn) NextReader() (messageType int, r io.Reader, err error) {
 	for c.readErr == nil {
 		frameType, err := c.advanceFrame()
 		if err != nil {
-			c.readErr = hideTempErr(err)
+			c.readErr = err
 			break
 		}
 
@@ -1073,7 +1065,7 @@ func (r *messageReader) Read(b []byte) (int, error) {
 				b = b[:c.readRemaining]
 			}
 			n, err := c.br.Read(b)
-			c.readErr = hideTempErr(err)
+			c.readErr = err
 			if c.isServer {
 				c.readMaskPos = maskBytes(c.readMaskKey, c.readMaskPos, b[:n])
 			}
@@ -1096,7 +1088,7 @@ func (r *messageReader) Read(b []byte) (int, error) {
 		frameType, err := c.advanceFrame()
 		switch {
 		case err != nil:
-			c.readErr = hideTempErr(err)
+			c.readErr = err
 		case frameType == TextMessage || frameType == BinaryMessage:
 			c.readErr = errors.New("websocket: internal error, unexpected text or binary in Reader")
 		}


Test patch:
diff --git a/conn_test.go b/conn_test.go
index f0c29c3..3bd4f61 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -814,3 +814,25 @@ func TestFormatMessageType(t *testing.T) {
 		t.Error("failed to format message type")
 	}
 }
+
+type fakeNetClosedReader struct {
+}
+
+func (r fakeNetClosedReader) Read([]byte) (int, error) {
+	return 0, net.ErrClosed
+}
+
+func TestConnectionClosed(t *testing.T) {
+	var b1, b2 bytes.Buffer
+
+	client := newTestConn(fakeNetClosedReader{}, &b1, false)
+	server := newTestConn(fakeNetClosedReader{}, &b2, true)
+
+	if _, _, err := server.NextReader(); !errors.Is(err, net.ErrClosed) {
+		t.Fatalf("server expects a net.ErrClosed error, %v returned", err)
+	}
+
+	if _, _, err := client.NextReader(); !errors.Is(err, net.ErrClosed) {
+		t.Fatalf("client expects a net.ErrClosed error, %v returned", err)
+	}
+}