Simple Websocket & HTTP client API support

[jukkar]

This PR provides a simple Websocket client API to send Websocket messages to Websocket server. Because the Websocket is established using HTTP, there is small and simple HTTP client API implemented. This HTTP client API can be used to connect to HTTP(S) servers so it is not tied to Websocket client implementation. As this HTTP API is very much needed by Websocket API, I decided not to send it separately to review.

The Websocket API provides functions that can be used by BSD socket API or if more fine grained Websocket support is needed, a Websocket specific API functions are provided.

The main function is this one

int websocket_connect(int http_sock, const char *host, const char *url,
		      const char **optional_headers,
		      websocket_connect_cb_t cb,
		      const struct http_parser_settings *http_cb,
		      u8_t *tmp_buf, size_t tmp_buf_len,
		      s32_t timeout, void *user_data);

It will be given a socket descriptor to the Websocket server. This means that all the non-websocket related things are done outside of this API. The websocket_connect() will connect to the server, do needed HTTP handshakes and return a new websocket socket descriptor that, when used, will add Websocket header to the sent data. User can then either use normal send() or websocket_send_msg() functions to send the data.

One usecase for this API is run MQTT data on top of it. This MQTT support is not part of this PR but to be done separately.

[jukkar]

Verified that http-client sample works with TLS. Added README files to both samples.

[rlubos]

Gone through most of the code, it looks that a lot of work was put into this!

[rlubos]

Any particular reason to use semaphore here instead of mutex?

[jukkar]

No particular reason for that, just re-using older code that used semaphore.

[jukkar]

Updated according to comments.

[dbkinder]

some suggested edits...

[dbkinder]

+1 for doc changes

[jukkar]

Fixed the handling of Sec-Websocket-Accept field in HTTP header.

[rlubos]

Thanks

[pfalcon]

Sorry, I was on vacation, then at the conference, so late to the party. Commenting first just based on the textual description and comments here.

The main function is this one

int websocket_connect(WALL OF PARAMETERS);

Didn't we discussed once that having gigantic parameter list is worse than a dedicated structure to capture those params instead? Some params are strange to, e.g.:

        websocket_connect_cb_t cb,

"sockets" and "callbacks" are generally don't mix together. How events are reported for sockets are via pull-style poll() call (instead of push-style callback). If you (for some reason) try to represent high-level websocket communication channel as a system socket, so callbacks, again, look strange.

        const struct http_parser_settings *http_cb,

Why structure of type "http_parser_settings" is named as if it was callback?

const char **optional_headers,

What if I don't have all the headers preallocated as single static array (e.g., don't have enough memory for that)?

all the non-websocket related things are done outside of this API. The websocket_connect() will connect to the server, do needed HTTP handshakes

The first part seems to contradict the second. So, does it issue connect() call? It shouldn't. And if doesn't, then I'd skip mentioning that it "will connect to the server". Mentioning "do needed HTTP handshakes" should be enough, perhaps elaborating it to "will do needed HTTP/Websocket handshake on already established connection".

and return a new websocket socket descriptor that,

If it returns file descriptor, that's our old good friend layering violation. Why conjugate high-level websocket connection to pretend it's a system-level socket. No other (sane) system does that. There're immediate portability implications, e.g. you can't test this websocket code on Linux with comfort it gives, so it's likely has bugs. And if it doesn't try to represent itself as a system-level socket, why return an int, instead of structure pointer?

when used, will add Websocket header to the sent data. User can then either use normal send() or websocket_send_msg() functions to send the data.

Hey, that's cool! But what about received data? ;-)

[pfalcon]

Fixed case when we received two websocket packets in same TCP segment, in that case we need to remember the second websocket msg for next socket read.

Websocket is SOCK_STREAM protocol, and SOCK_STREAM sockets don't work in terms of "TCP segments", they work in terms of "streams of bytes". So, if BSD Socket API was properly used to implement websocket code, this error couldn't happen in first place... I'm now a bit afraid to look into the actual code...

[pfalcon]

As mentioned earlier, callback paradigm is counter to native socket paradigm.

[pfalcon]

As mentioned earlier, callback paradigm is counter to native socket paradigm.

[pfalcon]

I'd say http parser settings is implementation detail of HTTP API, not interesting to the user, but who knows...

[pfalcon]

If this is do {} while (true), can you make it while (true) {}, because that's the standard idiom for infinite loop.

[jukkar]

Websocket is SOCK_STREAM protocol, and SOCK_STREAM sockets don't work in terms of "TCP segments", they work in terms of "streams of bytes". So, if BSD Socket API was properly used to implement websocket code, this error couldn't happen in first place... I'm now a bit afraid to look into the actual code...

The Websocket is a message protocol on top of stream protocol (TCP). IMHO it is not very well designed in the first place but we need to cope with this.

[jukkar]

Didn't we discussed once that having gigantic parameter list is worse than a dedicated structure to capture those params instead? Some params are strange to, e.g.:

Do you have a proposal how to pass the params?

Why structure of type "http_parser_settings" is named as if it was callback?

The struct contains HTTP callback pointers.

What if I don't have all the headers preallocated as single static array (e.g., don't have enough memory for that)?

That is not possible with this API. We would probably need to have a callback to feed extra headers to the stream.

all the non-websocket related things are done outside of this API. The websocket_connect() will connect to the server, do needed HTTP handshakes

The first part seems to contradict the second. So, does it issue connect() call? It shouldn't. And if doesn't, then I'd skip mentioning that it "will connect to the server". Mentioning "do needed HTTP handshakes" should be enough, perhaps elaborating it to "will do needed HTTP/Websocket handshake on already established connection".

There is no contradiction here, user is responsible to call connect(), the websocket API will then connect i.e., establish, like doing handshakes etc., a valid connection to websocket server.

and return a new websocket socket descriptor that,

If it returns file descriptor, that's our old good friend layering violation. Why conjugate high-level websocket connection to pretend it's a system-level socket. No other (sane) system does that. There're immediate portability implications, e.g. you can't test this websocket code on Linux with comfort it gives, so it's likely has bugs. And if it doesn't try to represent itself as a system-level socket, why return an int, instead of structure pointer?

We want to use socket descriptor that is tied to websocket connection. This helps to run things on top of websocket connection and use recv()/send() API. We are not making Linux applications here, and if you want to run valgrind etc you can use native_posix for that.

when used, will add Websocket header to the sent data. User can then either use normal send() or websocket_send_msg() functions to send the data.

Hey, that's cool! But what about received data? ;-)

recv() works just fine too

[jukkar]

Changing slightly the API proposal:

• moved most of the function parameters to user given request struct
• typos fixed
• added callback that allows user to send the http headers fields itself instead of giving them as pointers

[pfalcon]

Do you have a proposal how to pass the params?

Not something specific, beyond moving all (most) params to a struct, and passing a pointer to it to the function. I see that you already pushed something along those lines, let me have a look.

[pfalcon]

Is there a reason why user_data is passed as a separate param and is not part of struct http_request? (Just in case, I'm asking, not calling for change.)

[jukkar]

Many APIs in networking side etc. have typically timeout and user_data parameters in function calls. Just followed this style here.

[pfalcon]

Just followed this style here.

Makes sense.

[pfalcon]

I've reviewed this PR, and agree that it's useful and good deal of effort went into it. I appreciate reacting to some suggestions I've made. I have to say that I'm still not fully satisfied with it due to the fact that HTTP API uses callbacks instead of pull-style, socket-style read()/write() functions (as proposed in #13212). So, I'd give this +0.4, which rounds to +0.

At the same time, I understand why it's done like that - callbacks ultimately come from 3rd-party HTTP protocol parser we use, and "masking" them would just take extra effort, especially if the real task here was implementing Websocket support. The concern with callbacks is their known composability issues, but this very PR somehow counter-proves that, as it was possible to implement socket-style API for websocket on top of the underlying callback-style HTTP API (though it's unclear what issues may be lurking there) . So, overall, I don't think I'd come up with an overall better initial implementation even if I tried a callback-less route.

I definitely hope this will be merged soonish, to allow more people to play with it. In that regard, if my approval would help to merge it sooner rather than later, I'd be happy to give my +1 just for that reason, even though I'm not fully satisfied with the overall design.

[jukkar]

Noticed some typos and minor nits when doing self review. I will fix these.

[jukkar]

Noticed some fd leaks when closing the connection, those are now fixed.

[jukkar]

Seems to work ok now. I have been running mqtt_publisher over Websocket without any issues.