ClientCloseDecorator#close does not accept arguments

[PeterRunich]

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

	class ClientCloseDecorator < SimpleDelegator
	def initialize(client, connection)
	@client = client
	super(connection)
	end
	def close
	super
	if @client
	@client.close
	@client = nil
	end
	end
	end

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

	it "can connect to a websocket server and close underlying client" do
	Async do |task|
	connection = Async::WebSocket::Client.connect(client_endpoint)
	connection.send_text("Hello World!")
	message = connection.read
	expect(message.to_str).to be == "Hello World!"
	connection.close
	expect(task.children).to be(:empty?)
	end.wait
	end

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 :) )

[PeterRunich]

Okay, I figured out such a test, but it doesn't work with http 2. I'm going to continue my research.

PeterRunich@c48e6aa

[PeterRunich]

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
#<Protocol::HTTP2::Data Frame stream_id=1 flags=0 6b> #<Protocol::HTTP2::Data Frame stream_id=1 flags=0 2b>
as I understand it, this is the signal to close the connection
, but in the future, as I understood in the protocol-websocket library, the frames should turn into CloseFrame, but this does not happen.
Unfortunately, I am not yet familiar with the specification of http and websocket and do not understand the entire code base of the library very well. I hope this information will help @ioquatix figure it out.

[PeterRunich]

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
#<Protocol::WebSocket::CloseFrame:0x00007f78d33d1ff0 @finished=true, @flags=0, @opcode=8, @mask="\xA8%|\n", @length=2, @payload="\xAB\xCC">

[ioquatix]

Thanks for the investigation, I'll take a look later today.

[PeterRunich]

code https://github.com/PeterRunich/async-websocket/blob/de81530ab7b62fe76dcc39863afe0fa60425fda0/test/async/websocket/client.rb#L49-L79

Also, if you initiate the closing of the connection from the server on http2, something strange happen.

http1 outputs
1002

http2 outputs

1.29s     warn: Async::Task [oid=0xd34] [ec=0xd48] [pid=21689] [2023-03-07 12:01:10 +0300]
               | Task may have ended with unhandled exception.
               |   Protocol::HTTP2::ProtocolError: Cannot send data in state: closed
               |   → /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-http2-0.15.1/lib/protocol/http2/stream.rb:218 in `send_data'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-http-0.60.1/lib/async/http/protocol/http2/output.rb:119 in `send_data'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-http-0.60.1/lib/async/http/protocol/http2/output.rb:66 in `write'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-http-0.24.1/lib/protocol/http/body/stream.rb:130 in `write'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/frame.rb:218 in `write'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/framer.rb:59 in `write_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:93 in `write_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:199 in `send_close'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:87 in `rescue in read_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:68 in `read_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:230 in `read'
               |     test/async/websocket/client.rb:53 in `block (7 levels) in <top (required)>'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-2.4.0/lib/async/task.rb:107 in `block in run'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-2.4.0/lib/async/task.rb:248 in `block in schedule'
               |   Caused by EOFError: Could not read frame header!
               |   → /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/framer.rb:67 in `read_header'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/framer.rb:49 in `read_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:71 in `read_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:230 in `read'
               |     test/async/websocket/client.rb:53 in `block (7 levels) in <top (required)>'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-2.4.0/lib/async/task.rb:107 in `block in run'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-2.4.0/lib/async/task.rb:248 in `block in schedule'
 1.29s     warn: Async::Task [oid=0xd5c] [ec=0xd70] [pid=21689] [2023-03-07 12:01:10 +0300]
               | Task may have ended with unhandled exception.
               |   Protocol::HTTP2::ProtocolError: Cannot send data in state: closed
               |   → /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-http2-0.15.1/lib/protocol/http2/stream.rb:218 in `send_data'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-http-0.60.1/lib/async/http/protocol/http2/output.rb:119 in `send_data'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-http-0.60.1/lib/async/http/protocol/http2/output.rb:66 in `write'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-http-0.24.1/lib/protocol/http/body/stream.rb:130 in `write'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/frame.rb:218 in `write'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/framer.rb:59 in `write_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:93 in `write_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:199 in `send_close'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:87 in `rescue in read_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:68 in `read_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:230 in `read'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/3.2.0/delegate.rb:87 in `method_missing'
               |     test/async/websocket/client.rb:66 in `block (5 levels) in <top (required)>'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-2.4.0/lib/async/task.rb:107 in `block in run'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-2.4.0/lib/async/task.rb:248 in `block in schedule'
               |   Caused by EOFError: Could not read frame header!
               |   → /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/framer.rb:67 in `read_header'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/framer.rb:49 in `read_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:71 in `read_frame'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/protocol-websocket-0.10.0/lib/protocol/websocket/connection.rb:230 in `read'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/3.2.0/delegate.rb:87 in `method_missing'
               |     test/async/websocket/client.rb:66 in `block (5 levels) in <top (required)>'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-2.4.0/lib/async/task.rb:107 in `block in run'
               |     /home/pepe/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/async-2.4.0/lib/async/task.rb:248 in `block in schedule'

upd: I noticed that if you change the closing code (1001, 1002, 1003 etc), on the http1 server, the client still gets 1002 💀

[ioquatix]

Oops, looks a bit odd.

[ioquatix]

7eb22b6 fixes the argument passing, but it looks like we have other issues here.

[PeterRunich]

yep

[PeterRunich]

By the way, i use async-websocket in my library for discord and there the error codes come different correctly. There I use http1.

[ioquatix]

Hmm, I see part of the problem. The RFC 6455 is a bit weird:

If an endpoint receives a Close frame and did not previously send a
   Close frame, the endpoint MUST send a Close frame in response.  (When
   sending a Close frame in response, the endpoint typically echos the
   status code it received.)  It SHOULD do so as soon as practical.  An
   endpoint MAY delay sending a Close frame until its current message is
   sent (for instance, if the majority of a fragmented message is
   already sent, an endpoint MAY send the remaining fragments before
   sending a Close frame).  However, there is no guarantee that the
   endpoint that has already sent a Close frame will continue to process
   data.

   After both sending and receiving a Close message, an endpoint
   considers the WebSocket connection closed and MUST close the
   underlying TCP connection.  The server MUST close the underlying TCP
   connection immediately; the client SHOULD wait for the server to
   close the connection but MAY close the connection at any time after
   sending and receiving a Close message, e.g., if it has not received a
   TCP Close from the server in a reasonable time period.

   If a client and server both send a Close message at the same time,
   both endpoints will have sent and received a Close message and should
   consider the WebSocket connection closed and close the underlying TCP
   connection.

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.

[PeterRunich]

Maybe I misunderstood, but it says

the client SHOULD wait for the server to
close the connection but MAY close the connection at any time after
sending and receiving a Close message, e.g., if it has not received a
TCP Close from the server in a reasonable time period.

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.

[ioquatix]

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.

• Server sends data frames, but they take 100ms to travel through the network.
• In the mean time the client calls Connection#close.
• The client waits for the server to receive and echo the close frame.
• The client continues to receive data frames.
• The server receives the close frame and echoes it.
• The client eventually receives the close frame.

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.

[PeterRunich]

ok thanks, it doesn't really affect my code, I'll try to expand my knowledge, maybe I can help with something 🙂