[Bug] shutdown sends GOAWAY with reason internal error

[zuiderkwast]

Hi Loic,

when gun:shutdown/1 is called on HTTP/2, it causes a GOAWAY frame with reason INTERNAL_ERROR to be sent to the server. The expected message is GOAWAY with reason NO_ERROR.

I though I had implemented the shutdown handling in Gun, but I see now that you did it in #206. (I did it in cowboy.)

Anyway, I traced it as follows: gun:shutdown/1 → closing(status(State, shutdown), shutdown) → Protocol:closing(Reason, _, _, _), so gun_http2:closing/4 is called with reason 'shutdown', but only reasons 'normal' and 'owner_down' cause a GOAWAY frame with reason NO_ERROR. Any other reason (including shutdown) causes a GOAWAY frame with reason INTERNAL_ERROR.

closing(Reason0, State=#http2_state{socket=Socket, transport=Transport,
		http2_machine=HTTP2Machine}, _, EvHandlerState) ->
	Reason = case Reason0 of
		normal -> no_error;
		owner_down -> no_error;
		_ -> internal_error
	end,

The simple fix would be to add shutdown -> no_error here.

In gun_ws there is specific handling for reason 'shutdown' which suggests this is the right way to fix it.

gun_ws:closing(Reason, State, EvHandler, EvHandlerState) ...

closing(Reason, State, EvHandler, EvHandlerState) ->
	Code = case Reason of
		normal -> 1000;
		owner_down -> 1001;
		shutdown -> 1001;
		{error, badframe} -> 1002;
		{error, badencoding} -> 1007
	end,

I checked for tests covering this, but the gun initiated shutdown tests in shutdown_SUITE all use cowboy as the server, so the messages are not checked byte-by-byte.

[essen]

Both shutdown and {shutdown, _} should result in a NO_ERROR.

[zuiderkwast]

Protocol:closing/4 is only called from gun:closing/2 which is internal. gun:closing/2 is only ever called with 'shutdown' and 'owner_down' from within the same module. Thus, I believe handling of any other reason is dead code.

This 'closing' reason is not the same as the exit reason used elsewhere, for example in disconnect/2.

[essen]

Yes as far as the code is currently implemented only shutdown needs to be handled. But in the future it would be better to handle sys:terminate or EXIT signals properly and in that case both shutdown and the tuple should result in a NO_ERROR. But for now a smaller patch is fine.

[zuiderkwast]

To handle supervisor shutting down gun connections, EXIT and sys:terminate, we would need to use trap exit. I think it would be good to have, but it's a larger change. I made a PR with only a minimal fix.