Repository: gorilla/websocket
Pull request #865 (2023-11-09 10:43:51)

conn.go: default close handler should not return ErrCloseSent.
<!--
     For Work In Progress Pull Requests, please use the Draft PR feature,
     see https://github.blog/2019-02-14-introducing-draft-pull-requests/ for further details.

     For a timely review/response, please avoid force-pushing additional
     commits if your PR already received reviews or comments.

     Before submitting a Pull Request, please ensure that you have:
     - 📖 Read the Contributing guide: https://github.com/gorilla/.github/blob/main/CONTRIBUTING.md
     - 📖 Read the Code of Conduct: https://github.com/gorilla/.github/blob/main/CODE_OF_CONDUCT.md

     - Provide tests for your changes.
     - Use descriptive commit messages.
	 - Comment your code where appropriate.
	 - Squash your commits
     - Update any related documentation.

     - Add gorilla/pull-request-reviewers as a Reviewer
-->

## What type of PR is this? (check all applicable)

- [ ] Refactor
- [ ] Feature
- [x] Bug Fix
- [ ] Optimization
- [ ] Documentation Update
- [ ] Go Version Update
- [ ] Dependency Update

## Description

Noticed a change in default close handler from 1.5.0 to 1.5.1

1.5.1 Now returns the error from `WriteControl` with `CloseMessage` type. However this results in ErrCloseSent returned if the connection initiated the close handshake. This is normally correct as the connection should not be able to write after a close handshake is initiated, except when calling the close handler. in that case nil should be returned so that `advanceFrame()` can return the actual close message.

## Related Tickets & Documents

## 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

- [ ] `make verify` is passing (fails with `GO-2023-2186 Incorrect detection of reserved device names on Windows in path/filepath` that is unrelated to this change)
- [x] `make test` is passing


pennystevens: A similar change is required in the ping handler.  Consider this scenario:

1.  PING message is in flight from peer
2. the application writes a CLOSE message
3. the peer echos the CLOSE message
4. the PING message is received, connection responds with PONG
5. the CLOSE message is received, connection echos the CLOSE message.

This PR addresses step 5 where ErrCloseSent is returned instead of the CLOSE message.

Application never gets to step 5 because the PING handler returns ErrCloseSent and this error is returned from NextReader, ReadMessage, etc. 

In summary: When the connection automatically sends a message in response to a received message, ErrCloseSent should be ignored.  

Version 1.5.0 also ignored timeout errors in this scenario. 
 

pnx: @coreydaley can you take a look at this?
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/gorilla/websocket/pull/865?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla) Report
> Merging [#865](https://app.codecov.io/gh/gorilla/websocket/pull/865?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla) (07089bb) into [main](https://app.codecov.io/gh/gorilla/websocket/commit/629990daa33ae01adc1d520aa995487e1213bdc3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla) (629990d) will **increase** coverage by `0.20%`.
> The diff coverage is `50.00%`.

```diff
@@            Coverage Diff             @@
##             main     #865      +/-   ##
==========================================
+ Coverage   69.99%   70.20%   +0.20%     
==========================================
  Files          11       11              
  Lines        1593     1594       +1     
==========================================
+ Hits         1115     1119       +4     
+ Misses        364      362       -2     
+ Partials      114      113       -1     
```


| [Files](https://app.codecov.io/gh/gorilla/websocket/pull/865?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla) | Coverage Δ | |
|---|---|---|
| [conn.go](https://app.codecov.io/gh/gorilla/websocket/pull/865?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gorilla#diff-Y29ubi5nbw==) | `65.47% <50.00%> (+0.46%)` | :arrow_up: |



Patch:
diff --git a/conn.go b/conn.go
index 221e6cf..2a5ff76 100644
--- a/conn.go
+++ b/conn.go
@@ -1163,7 +1163,8 @@ func (c *Conn) SetCloseHandler(h func(code int, text string) error) {
 	if h == nil {
 		h = func(code int, text string) error {
 			message := FormatCloseMessage(code, "")
-			if err := c.WriteControl(CloseMessage, message, time.Now().Add(writeWait)); err != nil {
+			err := c.WriteControl(CloseMessage, message, time.Now().Add(writeWait))
+			if err != nil && err != ErrCloseSent {
 				return err
 			}
 			return nil


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 2b823dd..5b27f2f 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -477,6 +477,26 @@ func TestWriteAfterMessageWriterClose(t *testing.T) {
 	}
 }
 
+func TestWriteHandlerDoesNotReturnErrCloseSent(t *testing.T) {
+	var b1, b2 bytes.Buffer
+
+	client := newTestConn(&b2, &b1, false)
+	server := newTestConn(&b1, &b2, true)
+
+	msg := FormatCloseMessage(CloseNormalClosure, "")
+	if err := client.WriteMessage(CloseMessage, msg); err != nil {
+		t.Fatalf("unexpected error when writing close message, %v", err)
+	}
+
+	if _, _, err := server.NextReader(); !IsCloseError(err, 1000) {
+		t.Fatalf("server expects a close message, %v returned", err)
+	}
+
+	if _, _, err := client.NextReader(); !IsCloseError(err, 1000) {
+		t.Fatalf("client expects a close message, %v returned", err)
+	}
+}
+
 func TestReadLimit(t *testing.T) {
 	t.Run("Test ReadLimit is enforced", func(t *testing.T) {
 		const readLimit = 512