Handle send errors

[bjosv]

[To get faster reaction times on failing socket-send of echos/requests
we now handle the response from the send call where applicable.

To avoid a larger change in the statemachine a send error triggers an
event and is handled like a remote socket error/close.

Fixes #227 and touches #224

This PR is a proposal and should be seen as a base for discussing how to solve it the best way,
and ideas/comments are welcome.
(http and ws should have the same final solution, and tests can be cleaned up a bit aswell..)

[essen]

I suggest waiting for my HTTP/2 CONNECT (http2-connect branch) work to be complete because the socket will not always be a real socket and I suspect that this solution will not work in that case. I also dislike using a message, I'd rather return a command, though let's first see what the implications would be for proxies (TLS proxies may close the connection at the TLS level and this needs to be handled in a different way than when the real socket closes).

[zuiderkwast]

Hi! I'm collaborating with @bjosv on this. Do you want us to rebase this PR on the http2-connect branch?

This PR just addresses the fact that the return value from Transport:send/2 is ignored. I can't find anything related to that in the http2-connect branch. If the socket is not a real socket, Transport:send/2 can still return ok | {error, Reason} (since it's just what gen_tcp:send/2 and ssl:send/2 return, right?).

The message passing to self() is perhaps a bit ugly, but since most other errors from the transport come in the form of message passing, passing the error returned by send as a message to self makes gun handle it just like any other error, which is quite nice. Of course it would be better to abort sending a request once sending the headers fails, etc.

Can you clarify "I'd rather return a command"? Do you mean gun_http2:request, gun_http2:maybe_send_data, etc. should return something to trigger a new state in the gun state machine?

[essen]

Do not rebase against http2-connect, it's still undergoing significant changes.

For HTTP/2 CONNECT, yes the fake sockets still return errors (potentially, anyway), however the error has to be routed to the correct proxy layer (so if you have, for example, the socket at the end of the HTTP/2 CONNECT -> HTTP/2 CONNECT -> HTTP/1.1 returning an error, it must be handled by the middle layer, not by the innermost layer.

Can you clarify "I'd rather return a command"? Do you mean gun_http2:request, gun_http2:maybe_send_data, etc. should return something to trigger a new state in the gun state machine?

Yes. There is already an {error, _} command for example. But because of the large changes coming up it's best to wait a little before doing the work.

[essen]

Hello, I see you probably noticed that I merged the http2-connect branch. :-)

Note that I don't think sending ourselves a message is going to work well due to tunnels. I'm going to see if making all callbacks follow the {Commands, EvHandlerState} format would work (necessary for proper event handling as well), in which case an error command should be returned instead.

[essen]

I think work on this can be resumed. Like I've said earlier an error command should be returned. If it's in a callback that currently returns {State, EvHandlerState} it can "easily" be converted to {Commands, EvHandlerState}. I think the update_window callback would be in that case.

[zuiderkwast]

This one would be good to have in 2.0. I'll give it a try.

[zuiderkwast]

I've done a large rewrite now to propagate the errors as commands.

A command is returned instead of a state by these Protocol functions (already merged in #285):

• headers/12
• request/13
• keepalive/3
• data/7
• connect/9
• ws_upgrade/11
• cancel/5

In gun_http2, several functions may return a state or an error instead of only a state, e.g.

• frame/4 (called by parse/5 in the same module)
• maybe_ack_or_notify/2 (called by frame/4)
• data_frame/7 -> data_frame1/7 (called by frame/4)
• update_window/1 (called by frame/4, data_frame1/7)
• update_window/2 (called by update_flow/4, data_frame1/7)
• reset_stream/3
• push_promise_frame/7
• goaway/2
• maybe_send_data/6 -> send_data/4 -> send_data/6 -> send_data_frame/4
• tunnel_commands/5

Also gun_http and other modules are updated. I think "http2" can be removed from the PR name since send errors are handled for all protocols.

[essen]

Sounds good. But it's too big for me to look at today. If there's minor issues I will fix them during merge.

[essen]

There's more than minor issues. The todos need to be resolved. But perhaps you'd prefer for me to do the implementation for gun:cancel of tunnel streams? Because then you could use the same mechanism to abort tunnel streams when a send error occurs (only in that case it would be aborted because of a socket error).

[zuiderkwast]

perhaps you'd prefer for me to do the implementation for gun:cancel of tunnel streams? Because then you could use the same mechanism to abort tunnel streams when a send error occurs

Yes, that would be good. I must confess I'm a bit lost in the tunnels. I'd be happy if you can give me some pointers.

[essen]

There's a page in the user guide about the internals of the implementation. Please read it and tell me if this helps at all. I can improve while it's still clear in my mind.

[zuiderkwast]

Do you mean "TLS over TLS"? I have seen it but I don't fully understand how things should be propagated.

[essen]

Well the question is less about propagating and more about handling send errors also for the fake sockets, and what to do when that happens. Probably only stop the relevant stream for HTTP/2. But the entire connection for HTTP/1.1. Hence why I mention gun:cancel for tunnel streams, because then we know how to stop only tunnel streams.

[bjosv]

Hi @essen , any more thoughts about the gun:cancel for tunnel streams?

Also, do you have any thoughts about incorporating this in steps? I guess we could incorporate the error propagation support first, then add send-error support for each protocol in steps. Or do you see a scenario that hinders this?
Current behavior is that the socket finally gets closed and this will close all tunnels anyway, similar to the PR.

[essen]

Let's do this in steps. I have pointed out in review comments some of the bits that I think should be merged as a first step. The list is not exhaustive. Could you open a PR with these types of changes? They relate to returning commands instead of state. Any other change should be left for step 2 (details TBD).

[zuiderkwast]

Let's do this in steps. I have pointed out in review comments some of the bits that I think should be merged as a first step. The list is not exhaustive. Could you open a PR with these types of changes? They relate to returning commands instead of state.

Done. Please have a look here: #285.

[zuiderkwast]

Rebased.

[essen]

Thanks. Will go over this probably on Thursday.

[zuiderkwast]

FYI: Just rebased. No need to review.

[essen]

I have pushed a commit that handles HTTP/2 tunnel errors on top of the PR that was merged.

[zuiderkwast]

I have pushed a commit that handles HTTP/2 tunnel errors on top of the PR that was merged.

Awesome. I'll rebase this PR again onto latest master.

In gun_http2, several functions may return a state or an error instead of only a state, e.g. frame/4 (...)

Do you think I should rewrite all these to return {state, State} | {error, Reason}?

[essen]

I'm not sure, I was wondering about doing this with the tunnel changes but kept it simple. Considering the other changes you have in this PR (such as in gun_tunnel), perhaps this could be a good next step to normalize this in a separate PR. Let's retain the "StateOrError" variable names since they're not the full "Commands".

[zuiderkwast]

I have updated this PR after #301 was merged. Is there anything else that should be done in a separate PR or is this one small enough now?

[essen]

Almost there!

[essen]

Merged, thanks!