Skip to content

Commit

Permalink
conn.go: default close handler should not return ErrCloseSent. (#865)
Browse files Browse the repository at this point in the history
<!--
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

Co-authored-by: Corey Daley <cdaley@redhat.com>
  • Loading branch information
pnx and coreydaley committed Nov 13, 2023
1 parent 629990d commit 6f5d213
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 1 deletion.
3 changes: 2 additions & 1 deletion conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 20 additions & 0 deletions conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 6f5d213

Please sign in to comment.