search

dgrr / websocket

WebSocket for fasthttp
MIT License
59 stars 13 forks source link

Always close connection #3

Closed kenshaw closed 2 years ago

kenshaw commented 2 years ago

When using a "Normal" close status (ie, 1000), the close handler is not being called because Conn.closer was never closed. This commit fixes that.

kenshaw commented 2 years ago

This is another change to the logic. I believe the current logic is incorrect, as a "Normal" (1000) status should still call the registered Close handler. This was the simplest change that I saw to effect.

I believe the logic should be further refined/clarified as this:

  1. The error handler should only be called when there is a non-normal close
  2. The close handler should always be called when there is a close, regardless of the status

The PR changes at least one portion, but note that the Error handler might still be called with a nil error based on the current logic.

kenshaw commented 2 years ago

I just a different commit that eliminates the behavior of sending a nil error to the error handler. I believe this should now do what most developers would expect for the close/error handlers now. @dgrr please let me know if you have any feedback on this, thanks -- much appreciate the work on this package.

dgrr commented 2 years ago

Hello @kenshaw

Sending a nil error shouldn't happen. That means, if we receive a nil on the c.errch then it must be because it was closed. So I like your logic. And you are right, the onClose handle doesn't get called, that means there's a coroutine waiting on nothing.

Let me know if you are done with the PR so I can merge.

kenshaw commented 2 years ago

@dgrr I think for this specific issue, this can be closed merged/closed out. I'm still having trouble getting the mainline to work properly, where the 0.0.7 used to work fine, but I'm not sure if that's because I was using the 0.0.7 improperly or for some other reason. Feel free to leave this PR open, and if I do find other issues, I can push here. Otherwise, if you do merge/close it out, and there are other issues I find, I will of course open a new PR accordingly.

kenshaw commented 2 years ago

@dgrr I've fixed the unit tests as well. Apologies for going slowly with this, as it's a bit hard to isolate the specific places where I'm using this library. I'm not a websocket expert, so please let me know if the changes to the unit tests are correct or not.

Also, I've noticed that the nhooyr.io/websocket client, when using a Writer (and not a direct Write) with this package server-side, continuation frames don't seem to work properly.

dgrr commented 2 years ago

@kenshaw No worries. Thanks for your pr!