Skip to content
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

[WIP] Add websocket failure debug messages #2496

Closed
wants to merge 1 commit into from

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Feb 27, 2022

If a remote server sends a normal HTTP response without appropriate websocket headers then websocket open requests fail with HPE_CB_headers_complete error. This PR is intended to look at this, initially by adding an additional debug message at the point of failure to clarify what's happened.

How applications should handle this situation is perhaps more complicated.

See #2494 for discussion.

@slaff
Copy link
Contributor

slaff commented Feb 28, 2022

@mikee47 Do you want to add something more to this PR or we can leave it for another PR?

@mikee47
Copy link
Contributor Author

mikee47 commented Feb 28, 2022

The debug message is helpful for development, but I think the issue here is that the call to wsClient.connect() never gets a response. We need a failure callback, so related to #1939.

@slaff
Copy link
Contributor

slaff commented Mar 7, 2022

Let's do the following

  • Merge this PR as is

And then

  • Create a second PR which adds a second optional parameter to the bool connect(const Url& url); -> bool connect(const Url& url, TcpClientConnectCallback callback = nullptr); methods for all TCP clients.
  • A TcpClientConnectCallback has one parameter which is the id of the connection error.
  • A connection error can be:
    • Invalid parameters - invalid URL scheme
    • DNS error - domain not resolvable
    • TCP connection error - no IP and port to connect to
    • Protocol error - when the server side does not provide the expected response. For example when a websocket client tries to connect to a server and the server does not speak HTTP or speaks HTTP but does not provide the required headers

What do you say?

@mikee47
Copy link
Contributor Author

mikee47 commented Mar 7, 2022

I see what you mean, though only need callback for 'deferred' errors since invalid parameters, etc. will result in the connect() call failing immediately with a false return value.

The sample code is generally written with a call to connect() (without checking the return value) then immediately calling send(). If the connect fails, then it's not clear what happens with the queued request(s). A more logical approach might be:

auto connectionCallback = [&](bool success) {
  if(succcess) {
    client.send(...);
  }
};

if(!client.connect(url, connectionCallback)) {
  debug_e("Connect failed!");
}

So your approach of a per-connect callback is good but we can take the opportunity to provide a more logical and consistent way to use the framework.

In general, the framework doesn't both with error codes, just success/failure since the resolution is typically just to retry later and knowing the exact cause probably won't help much. Protocol clients could provide additional methods if it's useful.

NB. I've done some work on the websocket stuff and updated #1939 so may be of use.

@slaff slaff removed this from the 4.6.0 milestone Mar 9, 2022
@slaff
Copy link
Contributor

slaff commented Mar 9, 2022

Closing in favor of #1939.

@slaff slaff closed this Mar 9, 2022
@mikee47 mikee47 deleted the fix/websocket-errors branch January 15, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants