HTTP library that uses net-app

[jukkar]

This is a proposal for new HTTP library that uses net-app API instead of running things directly on top of net_context.
This supports both HTTP client and server functionality, both are also supporting TLS. Old HTTP API is deprecated.
I will also send a PR that will add websocket support to this new API.

[lpereira]

Although this won't crash, if buflen=NET_IPV4_ADDR_LEN, this will truncate this string if the address family is invalid.

[jukkar]

Sure, not a big issue thou as this is just used for debug printing.

[lpereira]

A future improvement could redirect all HTTP requests to HTTPS if TLS is enabled.

[jukkar]

Sure, but this depends on how application is doing things. If we have enabled HTTPS, then HTTP port is not normally listened at all. So application would need to create a listener to port 80 and then re-direct traffic to HTTPS port in this case.

[lpereira]

If the URLs array is sorted by URL length, it's possible to quickly perform a longest subprefix match without requiring fancier data structures like a trie. This should simplify URL matching as well.

[jukkar]

This can be done by application which can fill the url array in desired order.

[lpereira]

This is unused.

[jukkar]

Indeed, good catch.

[mike-scott]

I'll try and review the next version posted as I'm a bit buried atm.

[dbkinder]

add a comma before the "and" for the last item in this list.

[dbkinder]

s/like/such as/

[dbkinder]

add a comma before the "and" for the last item in this list.

[dbkinder]

s/must be/is/

[dbkinder]

s/must be understood as/is/

[dbkinder]

s/The inspection of/Inspecting/

[dbkinder]

some suggested edits

[nashif]

Get rid of all those http_app_xxx just call it http, there is no reason why we should have app in the name

[pfalcon]

It might happen in TCP client, that the TCP connection is terminated in which case net_context is freed. Check this and mark corresponding net_context inside net_app to NULL. This way there will be no issue to access already freed net_context.

This is very dangerous. What if it's freed and already reused?

Such condition should not happen, it's a bug and we should be fix it (which should be as simple as incrementing a refcount for it, or not so simple).

I'm writing in particular because I see some weird issues in MicroPython which could be explained by this issue to.

[jukkar]

What if it's freed and already reused?

That is why we mark the context as NULL here so that we do not reuse it in net-app.

I am not sure if adding refcount here would help as if TCP connection is closed, then we would need to inform the users of the net_context that it is closed. Probably using other flags in net_context we could figure this out in users of net_context.

[pfalcon]

That is why we mark the context as NULL here so that we do not reuse it in net-app.

What if it's reused by non-net-app?

I am not sure if adding refcount here would help as if TCP connection is closed, then we would need to inform the users of the net_context that it is closed.

Well, (at the very least) we just need to make sure that the closed context is properly marked so (so any further operation on it will return appropriate error), but it's not freed until user closes ("puts") its own reference to it.

[pfalcon]

As discussed in RFC on the mailing list and in review comments to #980, I really think that the new API doesn't offer enough improvements over old one, and even calls for particular, questionable design choices in the underlying stack (e.g. #1330).

So, I'd like to propose to rework it. Let me start with a first: reworking a TX path for HTTP.

Proposal:

1. There should be a single function to handle sending of HTTP content, let it be called http_send_data() (this reuses the name of function used in this PR for completely different semantics, hope that's not too confusing with this note.
2. It's prototype to be int http_send_data(struct http_ctx *ctx, const char *payload, size_t payload_len); Note - no references to network packets or something.
3. struct http_ctx should have a member struct net_pkt *tx_pkt, initially set to NULL.
4. Algorithm of the function described in below points.
5. If ctx->tx_pkt is not allocated, allocate it.
6. Add to the packet whatever data of payload fits. (This will require #119).
7. If all user data handled, return.
8. When full, send the packet to the network and set ctx->tx_pkt to NULL.
9. Repeat from 5. for remaining user data.

All other functions can be implemented in terms of http_send_data().

In addition to http_send_data(), should have http_flush_data(), which will send whatever data has already buffered (useful e.g. for websockets), and http_finish_data() (HTTP 1.0 would close connection here).

The names are tentative. Possibly, http_send_data() won't be user-facing function (likely wrappers will be required to be user-facing to abstract away chunked encoding), so that function can be called differently and "http_send_data" reused for more user-facing func.

[jukkar]

even calls for particular, questionable design choices in the underlying stack (e.g. #1330).

I do not understand how #1330 is related to this PR, could you elaborate this claim?

[pfalcon]

I do not understand how #1330 is related to this PR, could you elaborate this claim?

This PR introduces such an HTTP API which then requires #1330 to handle some of the API "aspects". If the API is redesigned, going for solution like #1330 won't be required (with many issues raised with it).

[jukkar]

This PR introduces such an HTTP API which then requires #1330 to handle some of the API "aspects".

This is false, the API does not mandate the use of #1330
I just wanted to simplify the sample application logic and avoid looping through data.

Anyway, I will investigate your proposal for API functions more next week.

[pfalcon]

Anyway, I will investigate your proposal for API functions more next week.

I appreciate that. I'll be happy to help with the implementation in any way I can.

[pfalcon]

So, I don't see http_client_send_post_req() in the code, only in the header, was anything done to it, or am I missing something?

Hmm, I do not recall any comment regarding that API, you want that changed too?

The comment is here: #4243 (comment) , I suggested that it's not general enough to not take the payload size (rules out use with binary payloads).

[jukkar]

The comment is here: #4243 (comment) , I suggest that it's not general enough to not take the payload size (rules out use with binary payloads).

Indeed. I will remove the POST helper, one is able to send a POST request without it anyway.

[pfalcon]

Makes sense, thanks!

[jukkar]

Changes:

• fix merge conflicts
• fix sanity errors
• removed POST req client helper
• moved functions around in http_app.c
• combined two chunk send functions together

[pfalcon]

Ok, I think this is a very good progress in the right direction. My general suggestion is that we treated such work as an iterative process, and didn't try to replace an older API version at once. For example, I may imagine, further changes/fixes will be required as this API gets more use.

But for iteration one it's really good, +1.

[jukkar]

@dbkinder the documentation checker fails with this message

/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/include/net/http_app.h:1040: warning: reached end of file while inside a verbatim block!
The command that should end the block seems to be missing!

/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/include/net/http_app.h:1040: warning: end of file while inside a group

I have spent several hours trying to understand what goes wrong but have failed so far. The header file contains proper @{ and @} markers so it baffles me what is the root cause of this warning. And doxygen does not tell what is the actual problem it encountered. Could you help here to solve this issue?
I also verified that the "make htmldocs" generates proper documentation for the HTTP api.

[jukkar]

@nashif Ping, does this look something that is acceptable to you? I deprecated the old API, but if you want I can certainly remove it alltogether.

[dbkinder]

@jukkar @nashif It took quite a while but I found the culprit. I ended up just deleting chunks of lines, running doxygen on just this file, and finding the line(s) causing the problem. Doxygen doesn't like to see \r in a doxygen comment. You've got a few in here as \r\n that I deleted (leaving just the \n) the the verbatim block and end of file in a group warning went away, yea! Maybe replace use of \r\n in doxygen comments as HTTP_CRLF (as in the #define you have).

That then exposed some errors because you missed the documentation for a couple of function parameters:

http_app.h:967: warning: The following parameters of http_add_header(struct http_ctx *ctx, const char *http_header_field, void *user_send_data) are not documented:
parameter 'user_send_data'

http_app.h:983: warning: The following parameters of http_add_header_field(struct http_ctx *ctx, const char *name, const char *value, void *user_send_data) are not documented:
parameter 'user_send_data'

[dbkinder]

\r in doxygen comments can cause odd "end of file while inside a verbatim block" warnings

[dbkinder]

Can't use \r in a doxygen comment. Recommend something like this instead:

 The format is "name: value<HTTP_CRLF>"

[jukkar]

Thanks David! Really peculiar issue indeed, especially as the warning did not match the root cause.

[pfalcon]

Can't use \r in a doxygen comment.

Maybe it should be double-quoted, like \\r? @jukkar , please try that first, because having \r\n is clearer than <HTTP_CRLF>.

[jukkar]

Changed the documentation so that sanity check passes without warnings.

[dbkinder]

If we're starting a trend to ignore *_legacy.h files, better would be to delete these individual files listed here and change the line below to be:

 EXCLUDE_PATTERNS = *_legacy.h

[jukkar]

I am ok with this. Perhaps this could be a separate patch and not part of this set.

[dbkinder]

Ok. I’ll submit a separate patch for ignoring all _legacy.h files.

[dbkinder]

I've verified that using \\r\\n works in doxygen, so if you'd like to use that, I think the output would be nicer, though the way you've written this works for me since it references the #define above.

[jukkar]

Ok, I think too that \r\n looks better in the documentation. I will send a new version that changes this.

[jukkar]

Updated the documentation and changed <HTTP_CRLF> to \\r\\n

[jukkar]

Fixed the http_url_flags enum values in header file.