Web socket over http2 issue

[chesskingaka]

I have ve two issues for discussion to see if we can make the changes for that.

1. Race condition issue
   before I made the test as step below. (Cowboy:start_clear with enable_connect_protocol=true was called before, http2 as protocol)

{ok, ConnPid} = gun:open("localhost", config(port, Config), #{
		protocols => [Protocol],
		http2_opts => #{notify_settings_changed => true}
	}),
{ok, Protocol} = gun:await_up(ConnPid),
{notify, settings_changed, #{enable_connect_protocol := true}}
		= gun:await(ConnPid, undefined),
StreamRef = gun:ws_upgrade(ConnPid, "/", []). ....

And I see that the gun_ws_upgrade function was failed with an error: "Stream was reset by server" after investigation, look like the issue is the value enable_connect_protocol in local_settings still not updated yet ( The value of enable_connect_protocol in local_settings is used for verifying, not remote_settings field in #http2_machine)

headers_pseudo_headers(Frame, State=#http2_machine{local_settings=LocalSettings},
		Type, Stream, Headers0) when Type =:= request; Type =:= push_promise ->
	IsExtendedConnectEnabled = maps:get(enable_connect_protocol, LocalSettings, false),
	case request_pseudo_headers(Headers0, #{}) of
		%% Extended CONNECT method (RFC8441).
		{ok, PseudoHeaders=#{method := <<"CONNECT">>, scheme := _,
			authority := _, path := _, protocol := _}, Headers}
			when IsExtendedConnectEnabled ->
			headers_regular_headers(Frame, State, Type, Stream, PseudoHeaders, Headers);
		{ok, #{protocol := _}, _} -> /// ***** The code come to this case****
			headers_malformed(Frame, State,
				'The :protocol pseudo-header is only defined for the extended CONNECT. (RFC8441 4)'); 

And I see that after sending the ACK_SETTING frame, the enable_connect_protocol value will be passed to local_settings from remote_settings

maybe_ack_or_notify(State=#http2_state{reply_to=ReplyTo, socket=Socket,
		transport=Transport, opts=Opts, http2_machine=HTTP2Machine,
		pings_unack=PingsUnack}, Frame) ->
	case Frame of
		{settings, _} ->
			%% We notify remote settings changes only if the user requested it.
			_ = case Opts of
				#{notify_settings_changed := true} ->
					ReplyTo ! {gun_notify, self(), settings_changed,
						cow_http2_machine:get_remote_settings(HTTP2Machine)};
				_ ->
					ok
			end,
			case Transport:send(Socket, cow_http2:settings_ack()) of
				ok -> {state, State};
				Error={error, _} -> Error
			end;

My concern is can we change the order of the "sending setting_ack frame" function and "sending gun_notify" function to avoid this issue, since if the gun:ws_upgrade was called before the value of enable_connect_protocol is updated succesfully in local_settings , the function will failed.

2. Should we implement gun:await/1?
   gun:wait/2 now need the stream_refs as second parameter, in case of "gun_notify" we don't have stream_refs (also stream_refs not used), so gun:wait with stream_refs = undefined can be used, but it can be lead to the dialyzer issue

[essen]

For 1. can you provide the error logs as well as the Cowboy setup? What you did is exactly what the test suite does so I'm a bit surprised you get a race condition. Plus since we always send the ack before handling the ws_upgrade there shouldn't be any problem.

For 2. I thought about it and there's a todo but I'm not sure. I'd like to see what it will look like for HTTP/3 (which I'm working on) and perhaps WebTransport.

[chesskingaka]

1. I made the test when working on new requirement for the product at my company, so sorry I cannot share it. in the test after received the gun_notify , I will send the gun:ws upgrade right away, ack also send right await after gun_notify is sent, so I think race condition can be happened?

BTW, I see the data in gun_notify include the value of enable_protocol_connect which get from remote_settings while the value of enable_protocol_connect in local_settings is used to verify when handling ws_upgrade, I have a bit confuse here.

in my test, if I put timer:sleep (100ms ) before gun:ws_upgrade, it will upgrade successfully.

2. Thanks for your information, In my company product, I cannot use gun:wait(ConnId, undefined) due to dialyzer issue. But maybe I can try make local changes in my product.

[essen]

1. The Gun process will not handle the ws_upgrade message until after it has sent the SETTINGS ack. If there is a race condition it's not likely to be here. I need a way to reproduce because as I mentioned the Gun test suite does what you do and never had any sort of race condition. See https://github.com/ninenines/gun/blob/master/test/ws_SUITE.erl#L61-L67 (and https://github.com/ninenines/gun/blob/master/test/ws_SUITE.erl#L268-L269) and other tests in this test suite but there's also more in the Cowboy test suite.

2. You can silence the warning since you know it's not a bug.