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

Close description can be un-initialized at time #33

Closed
bsergean opened this issue Apr 17, 2019 · 8 comments
Closed

Close description can be un-initialized at time #33

bsergean opened this issue Apr 17, 2019 · 8 comments

Comments

@bsergean
Copy link
Collaborator

We can see this easily by running continuously make ws_test. In that run below it it set to Normal closure / but at other times it is set to '%'.

@Dimon4eg + @Kumamon38 / This is likely a regression of 935e679

I think this is why I was only setting the close reason only once in a weird way, like the first time it gets sets in one of the callbacks.

Received 61 bytes
ws_transfer: 0 bytes left to be sent
ws_send: received message (61 bytes)
Done !
ws_send: connection closed: code 1000 reason Normal closure

Closed connection code 1000 reason %
WebSocketServer::handleConnection() done
Received file does exists, exiting loop
1897227451 20480000 /tmp/ws_test/send/20M_file
1897227451 20480000 /tmp/ws_test/receive/20M_file
Done !
Closed connection code ws_receive: connection closed: code 1000 reason Normal closure
0
 reason 
WebSocketServer::handleConnection() done

another run

ws_send: received message (61 bytes)
ws_send: connection closed: code 1000 reason Normal closure

Closed connection code 1000 reason ??
WebSocketServer::handleConnection() done
Received file does exists, exiting loop
4293923412 20480000 /tmp/ws_test/send/20M_file
4293923412 20480000 /tmp/ws_test/receive/20M_file
Done !
ws_receive: connection closed: code 1000 reason Normal closure
Closed connection
 code 1000 reason ac
@bsergean
Copy link
Collaborator Author

I'll take a look when I have time but if you have an idea for a fix please go ahead :)

@bsergean
Copy link
Collaborator Author

ps: I should say that I am not positive that it is a regression, but it is still a bug.

@AlexandreK38
Copy link
Contributor

AlexandreK38 commented Apr 18, 2019

I had a look and created a PR to fix this, but in fact this was nothing to do with the 935e679

  • First, when we receive the close code / reason remotely, the reason had 2 characters more than it needed (that why we could see "ac" or "%" or whatever randomnly read from memory)
  • Second, when we used the close() method, we sent the code but not the reason, so we can't expect to read it on the other side
  • Not directly linked, but we can call the _onCloseCallback(_closeCode, _closeReason, _closeWireSize); with not really initialized value (0, "", 0) in the case we call setReadyState(CLOSED) outside the close() method

All of this should be fixed in PR #34

@AlexandreK38
Copy link
Contributor

So is the issue fixed as you wanted with last PR?

@bsergean
Copy link
Collaborator Author

make ws_test seems to behave now !

@bsergean
Copy link
Collaborator Author

I think that the next step here is to add the ability to specify a reason and a code when calling websocket.close, but that might be very easy.

@AlexandreK38
Copy link
Contributor

Just 2 parameters to add; you can if you want to :)

@bsergean
Copy link
Collaborator Author

bsergean commented Apr 18, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants