new HTTP API bugfixes

[mike-scott]

• fixup http_client sample and include which still has CRLF being appended to protocol in places
• honor the CONFIG_HTTP_CLIENT_NETWORK_TIMEOUT setting in net_app_client.c
• remove the HTTP client chunked payload processing as it's currently broken and not compatible with Content-Length header setting

[mike-scott]

Patch 2: Updated the function description for http_prepare_and_send() in include/net/http_app.h

[pfalcon]

From the commit message:

However, http_send_chunk() needs to return the total bytes added to the packet so that the for loop in http_request() can monitor how much of the payload has been processed.

I told @jukkar in the review that the for loop in http_request() is not needed. And now it indeed fires back. So, the proper fix is IMHO to remove it.

The API contract of http_prepare_and_send() is very simple: it can have only 2 outcomes: a) it sends out all the data requested; b) it finished with an error.

Errors aren't recoverable, yeah. If we want to make (some) errors recoverable, then yes, we'd need to return the (partial) amount of data sent before an error occur.

I'd recommend against that, at least at the current design phase. We'd have a nice blocking server paradigm, where concurrency is handled by threads. If we add recoverable errors (well, there can be only one from user's perspective - EAGAIN), we effectively go into the land of non-blocking servers with all the complications that brings.

Let's do it step by step. To remind, there doesn't seem to support of multiple concurrent requests for now at all, let's deal with that first. (I remember @jukkar submitted patch to add concurrent requests to net_app_server, all that needs to be thoroughly reviewed/propagated yet.)

[mike-scott]

Can we agree that we need to:

• Remove my modifications to http_prepare_and_send() return value
• Remove the for loop in http_request()
• Remove the chunk_header info at the beginning of http_send_chunk() as this conflicts with the Content-Length header we are setting when payload (https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/net/lib/http/http_app_client.c#L132).

Per https://tools.ietf.org/html/rfc7230#section-3.3.1 "A sender MUST NOT send a Content-Length header field in any message that contains a Transfer-Encoding header field." -- And by extension of that Transfer-Encoded header information.

[mike-scott]

Actually we can remove http_send_chunk() usage from http_app_client.c and replace with call to http_prepare_and_send().

[mike-scott]

So maybe leave the http_send_chunk() function intact for usage w/ http_app_server.c

[pfalcon]

Per https://tools.ietf.org/html/rfc7230#section-3.3.1 "A sender MUST NOT send a Content-Length header field in any message that contains a Transfer-Encoding header field."

Aha, thanks for researching that. Well, kinda makes sense, if Content-Length is known, there's not point to use 'Transfer-Encoding: chunked" in the first place. The only usecase for it is when Content-Length is not known beforehand, and we want to keep the persistent connection.

[pfalcon]

Can we agree that we need to:

Well, definitely needs feedback from @jukkar, so not before next Mon, and next week is pre-freeze week, and I don't think we can solve all the issues with new HTTP API, which are many. So maybe, spend more time to better design it in 1.11, and keep the old HTTP API in 1.10.

[pfalcon]

Actually we can remove http_send_chunk() usage from http_app_client.c and replace with call to http_prepare_and_send().

Hackish workaround for a particular usecase. There're 2 usecases: 1) sending predetermined amount of data; 2) sending dynamically generated data (of length unknown at the time of the generation start). We need to support both, and for both client and server. (If we want a generic HTTP API, not a weird collection of arbitrary restrictions.)

[nashif]

Well, definitely needs feedback from @jukkar, so not before next Mon, and next week is pre-freeze week, and I don't think we can solve all the issues with new HTTP API, which are many. So maybe, spend more time to better design it in 1.11, and keep the old HTTP API in 1.10.

those are fixes and fully ok to be addressed for 1.10

[mike-scott]

@pfalcon Are you saying http_send_chunk() is a hackish work around? Or removing it from http_request() is a hackish work around?

I'm attempting to put this API into a state where it works at least as good as the legacy API.

The old API didn't do chunked data sending at all. And, I feel like there will be quite a bit of discussion before an appropriate change can be made to the new API to support your usecase #2.

[pfalcon]

@pfalcon Are you saying http_send_chunk() is a hackish work around? Or removing it from http_request() is a hackish work around?

The phrase quoted was a workaround, specifically "can remove call to X and replace with call to Y".

The old API didn't do chunked data sending at all.

Really? I believe it did ;-).

[pfalcon]

@nashif:

those are fixes and fully ok to be addressed for 1.10

If I wasn't clear in my comments above, we have problems with HTTP API design and implementation. Fixing design problems in a rush is usually not possible, requires stepping back and consideration.

[mike-scott]

Patchset #3 enables us to move to the new API and retain all of the current functionality.

[mike-scott]

@pfalcon Yes, but in the meantime what are new users of Zephyr going to use.. the depreciated API? I doubt will even give it a look.

I think the new API needs to be fixed now for 1.10, and then we can expand on the API going forward.

NOTE: I said "fixed". And by that, I mean let's get the same functionality working under this new API as what was working with the old API.

[mike-scott]

Really? I believe it did ;-).

@pfalcon I believe this is the exact area you would be looking for: https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/net/lib/http/http_client.c#L153
And I don't see any method for this to iterate through the payload in a chunked process.

[pfalcon]

but in the meantime what are new users of Zephyr going to use.. the depreciated API? I doubt will even give it a look.

Well, I guess we need to get our expectations right. #4840 was submitted, based on the project core maintainer request, which removes an existing HTTP API. So, the only expectation behind such a request may be that after all these years (2 or so) nobody uses Zephyr HTTP API.

Now you're saying "new users ... will use ...". If our expectation is that nobody have been using the API for 2 years, then next expectation should be that nobody is going to use it for some time yet. So, I wouldn't worry too much about that, and rather worry how to get good API when somebody starts to use it (soon, but not a week's time).

NOTE: I said "fixed". And by that, I mean let's get the same functionality working under this new API as what was working with the old API.

My proposal is different: we already have one semi-working HTTP API, why do we need another semi-working API (which will likely need to be deprecated again to get the right API). Let's stay with the old API (can un-deprecate it, easy), and do a good homework to make the new API truly better.

Note that it's just my proposal.

[pfalcon]

I believe this is the exact you would be looking for: https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/net/lib/http/http_client.c#L153

Instead, I always looked at https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/net/lib/http/http_server.c#L51 .

So, we're even talking about about different things - you seem to have meant client support all the time (but the title of this ticket doesn't mention it, nor #4882 (comment) specifically does), while I was talking with the server in mind.

That's why I propose to step back, collect requirements, implement them. I may be wrong, that's just a proposal ;-).

[mike-scott]

Well, I guess we need to get our expectations right. #4840 was submitted, based on the project core maintainer request, which removes an existing HTTP API. So, the only expectation behind such a request may be that after all these years (2 or so) nobody uses Zephyr HTTP API.

Not true. We've been using the Zephyr HTTP API for quite some time (over a year).

Now you're saying "new users ... will use ...". If our expectation is that nobody have been using the API for 2 years, then next expectation should be that nobody is going to use it for some time yet. So, I wouldn't worry too much about that, and rather worry how to get good API when somebody starts to use it (soon, but not a week's time).

Moot point. People are using the APIs. To expect otherwise would be counter-productive to why the APIs were put in the first place. We kind of made the bed on this one and we need to at least tuck in the sheets before deciding how to improve the mattress.

You said above we needed to approach this with KISS methodology. First step is fix the broken. If we need yet another NEW API that then introduces all of the improved functionality, then so be it.

My proposal is different: we already have one semi-working HTTP API, ...

It's simple: it's depreciated. No one is going to build new code using it. And existing users (like us) are already in the process of migrating for 1.10.

[mike-scott]

@pfalcon And that's the entire reason I left http_send_chunk() in place -- so the new server code can still use it. If you would like I can run a series of validation tests on the HTTP server functionality too.

NOTE: I'll update the title of the 3rd patch to include "client".

[pfalcon]

Not true. We've been using the Zephyr HTTP API for quite some time (over a year).

And that's the problem I'm trying to solve:

1. Stop removing APIs used by people.
2. Stop replacing one semi-working APIs with another semi-working.

It's simple: it's depreciated. No one is going to build new code using it. And existing users (like us) are already in the process of migrating for 1.10.

Instead, we could have made a point to the maintainers re: (at least) the p.1 above.

I agree with all the other things you say, will be happy to remove my -1 when approved by someone else (as you know, in github, you can't change -1 to +0).

[mike-scott]

Updated the 3rd patch title and description. No code changes.

[jukkar]

@mike-scott fixes makes sense so +1 to those.
We can continue API removal discussion in #4840 otherwise this PR becomes too convoluted.

[jukkar]

As there is now some changes done for HTTP APIs, could anyone also review #4859