Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write doesn't check if connection is closed before returning error #215

Closed
zhangyoufu opened this issue Mar 15, 2020 · 6 comments
Closed
Labels
Milestone

Comments

@zhangyoufu
Copy link

zhangyoufu commented Mar 15, 2020

If Conn.Close is called when there's on-going read/write, read/write may return I/O error instead of wrapping a CloseError, and there is no trivial way to examine whether this I/O error is caused by calling Conn.Close.

Of course I can record the code and reason before I call Conn.Close, but there is still a race between my call to Conn.Close and something like WebSocket protocol error. My recorded code and reason may not reflect what actually sent to remote peer in the close frame.

How to reproduce

package main

import (
	"context"
	"log"
	"time"

	"nhooyr.io/websocket"
)

func work(ctx context.Context) (err error) {
	client, _, err := websocket.Dial(ctx, "ws://demos.kaazing.com/echo", nil)
	if err != nil { return }
	log.Print("connected")

	// write something in background
	errCh := make(chan error)
	go func() {
		for {
			err := client.Write(ctx, websocket.MessageText, []byte("hello"))
			if err != nil {
				errCh <- err
				return
			}
			time.Sleep(50 * time.Millisecond)
		}
	}()

	// close after some time
	time.Sleep(3 * time.Second)
	log.Print("closing")
	err = client.Close(websocket.StatusNormalClosure, "bye")
	if err != nil { return err }
	log.Print("closed")

	return <-errCh
}

func main() {
	err := work(context.Background())
	if err != nil && websocket.CloseStatus(err) != websocket.StatusNormalClosure {
		log.Fatal(err)
	}
}

Expected output

2020/03/15 14:14:00 connected
2020/03/15 14:14:03 closing
2020/03/15 14:14:03 closed

Actual output (sometimes, due to race)

2020/03/15 14:13:54 connected
2020/03/15 14:13:57 closing
2020/03/15 14:13:58 failed to close WebSocket: failed to read frame header: read tcp 192.168.1.2:57362->34.209.17.111:80: read: connection reset by peer
@nhooyr
Copy link
Contributor

nhooyr commented Mar 15, 2020

Is your echo server written with this library?

The error indicates the library did not receive a close frame from the peer.

@nhooyr
Copy link
Contributor

nhooyr commented Mar 15, 2020

Will note that I should add an error wrap in https://github.com/nhooyr/websocket/blob/ff876f6d14d96bf1de6094887d6bbf167991b046/close_notjs.go#L83 to make that clear.

@zhangyoufu
Copy link
Author

zhangyoufu commented Mar 15, 2020

Is your echo server written with this library?

The echo server used is hosted by 3rd party. (It has some strange behavior on continuation frame, but not related to this issue.)

The error indicates the library did not receive a close frame from the peer.

Yes, you're right. Actually this code does not correctly expose what I want to show. I'll rewrite the code and post later.

@zhangyoufu
Copy link
Author

The code I provided does not show the problem clearly. I'm going to close this issue and open another.

@nhooyr
Copy link
Contributor

nhooyr commented Mar 15, 2020

All in progress reads and writes will do what you describe. But the close itself will return an IO error if it occurred as that means the close handshake wasn’t done.

@nhooyr
Copy link
Contributor

nhooyr commented Mar 15, 2020

Nvm it looks like I missed this on writes when I recently refactored the library. Thanks for reporting will fix soon.

@nhooyr nhooyr reopened this Mar 15, 2020
@nhooyr nhooyr changed the title read/write may not return CloseError when Conn.Close is called Write errors don't check if connection is closed before returning Mar 17, 2020
@nhooyr nhooyr added bug and removed needs-info labels Mar 17, 2020
@nhooyr nhooyr changed the title Write errors don't check if connection is closed before returning Write doesn't check if connection is closed before returning error Mar 17, 2020
@nhooyr nhooyr added this to the v1.8.5 milestone Apr 13, 2020
@nhooyr nhooyr closed this as completed in 2dc66c3 Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants