-
-
Notifications
You must be signed in to change notification settings - Fork 17
ClientCloseDecorator#close does not accept arguments #50
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
Comments
Okay, I figured out such a test, but it doesn't work with http 2. I'm going to continue my research. |
I haven't been able to figure it out yet, but when I send #close 1001, then in the server code on this line https://github.com/socketry/protocol-http2/blob/main/lib/protocol/http2/framer.rb#L82 (using print) you can catch two frames |
if you switch to http 1 then here https://github.com/socketry/protocol-websocket/blob/58f0d15c6c5b44d224fd7a6eb237ce331bd8a587/lib/protocol/websocket/framer.rb#L54 You can catch the CloseFrame I was talking about |
Thanks for the investigation, I'll take a look later today. |
Also, if you initiate the closing of the connection from the server on http2, something strange happen. http1 outputs http2 outputs
upd: I noticed that if you change the closing code (1001, 1002, 1003 etc), on the http1 server, the client still gets 1002 💀 |
Oops, looks a bit odd. |
7eb22b6 fixes the argument passing, but it looks like we have other issues here. |
yep |
By the way, i use async-websocket in my library for discord and there the error codes come different correctly. There I use http1. |
Hmm, I see part of the problem. The RFC 6455 is a bit weird:
When you receive a close frame, you should echo it back, but this often fails because the stream is closed. I wonder what the correct behaviour is here. |
Maybe I misunderstood, but it says the client SHOULD wait for the server to is it waiting for any signal from the server on the client now when I call #close? I haven't looked at the source code, but judging by the way I see it, I guess it just sounds like close and the connection closes immediately. |
I'm not sure there is value in waiting for the server to send a close frame, unless it's a matter of processing any pending data frames, i.e.
In terms of code, this would need to be something like this: # On the server:
server.send(...) # send data frames
server.close # send close frame
# On the client:
client.send_close # send the close frame to the server
while message = client.read # Eventually this will return nil
process(message)
end I'll continue investigating this tomorrow. |
ok thanks, it doesn't really affect my code, I'll try to expand my knowledge, maybe I can help with something 🙂 |
I've released the main fix (passing arguments through to close) in v0.25.0. Can you please try it out. I believe it should correctly handle closed states and |
#close arguments work :) on server rescue Errno::EPIPE => e
changed to
rescue Protocol::WebSocket::ClosedError => e i think it's good https://github.com/PeterRunich/async-websocket/blob/oddities-with-closing/test/async/websocket/wip.rb i read a little more about error codes |
traces for second test: http1
http2 sleep 1 not commented
http2 sleep 1 commented
|
just in case, I repeat, this applies to the first test with http2 |
Off-topic: |
Hi again!
I opened this #46 issue some time ago.
Today i found a small bug, we forgot to forward arguments in the delegator #close method.
async-websocket/lib/async/websocket/client.rb
Lines 38 to 52 in cff18c0
Here are the arguments I was talking about
https://github.com/socketry/protocol-websocket/blob/58f0d15c6c5b44d224fd7a6eb237ce331bd8a587/lib/protocol/websocket/connection.rb#L62-L63
PeterRunich@3e7698d
i fixed it, but i didn't figure out how to write a test.
i think it should look similar like this test
async-websocket/test/async/websocket/client.rb
Lines 23 to 33 in cff18c0
but i didn't figured out how to expect "moment of closing" on the server and comparing close codes, it would be nice if you explain it (or if it takes less of your time, you can write it yourself :) )
The text was updated successfully, but these errors were encountered: