gopherjs / websocket

websocket provides high- and low-level bindings for the browser's WebSocket API.
BSD 3-Clause "New" or "Revised" License
111 stars 25 forks source link

don't drop CloseEvent silently #12

Closed cryptix closed 7 years ago

cryptix commented 9 years ago

Hi,

currently the event object of the close event is silently ignored and dropped. I use a Conn from this package to build a net/rpc client and stumbled over this when I saw that uses of a closed connection returned an empty error. I can create a more verbose reproduction if needed.

Furthermore, the reason string was empty but the code was set (for me at least on Linux with Chromium42 and Firefox37). I found a post on stackoverflow which had compiled the code>reason mapping from RFC6455.

I think a proper fix should also fix or improve #7 but I wanted feedback first on how to progress on both cleanly.

mjohnson9 commented 9 years ago

This is a good idea, as the current implementation provides no more information than EOF. I was actually thinking of switching to using an io.Pipe (it has Close and CloseWithError) now that they work.

I'll review the current changes.

@dominikh What do you think?

cryptix commented 9 years ago

I converted the string lookup to a map. It does look a lot cleaner.

I kept the one console.Error of the wrapped event in until we find a better way to expose it. io.Pipe sounds like a good solution.

cryptix commented 9 years ago

I tried io.Pipe() and it is functional but I don't get the close error exposed when using the Conn again. The error is still just an empty object.

Maybe I don't understand the receiveFrame() > handleFrame() flow fully.

I'll setup a simpler test app that is gopherjs serveable to get the sourcemaps working.

dmitshur commented 7 years ago

What's the status here?

This PR seems quite out of date, should we close it, or is there anything to do here?

cryptix commented 7 years ago

Agreed