Re: [lwip-users] httpc_get_file doesn't seem to download entire file?

[Simon Goldschmidt]

Am 16. Juni 2021 20:15:02 MESZ schrieb "Rémy DZIEMIASZKO" 
<remy.dziemiaszko@smile.fr>:
>Le mer. 16 juin 2021 à 15:24, Bas Prins <bas.prins3@gmail.com> a écrit
>:
>>
>> Hi Remy,
>>
>> Thanks for your answer.
>>
>> It feels a bit awkward this way though. Do you know any reason why
>user code should be bothered with reloading the timeout ticks? Why
>doesn't http_client deal with this when it gets notified about a new
>packet on "httpc_tcp_recv" ?
>>
>
>I agree with you that http_client timeout management shall be improved:

Definitely seems so. Would anyone please file a bug report for this? Or even 
better, a patch fixing this? The timeout should be reset when receiving a chunk 
but should probably be overridable at compile time or runtime, too.

Regards,
Simon

>1) By reloading the timer after each chunk reception (what you have
>implemented below)
>2) By giving the possibility for the application to choose the timeout
>value(s). From simple to more complex possible enhancement:
>2.a) By turning HTTPC_POLL_TIMEOUT into a lwip option instead of a
>fixed #define.
>2.b) By passing an additional parameter to httpc_get_file to configure
>for each connection a specific timeout value (because timeout value
>may depend on the server behaviour or network performance).
>2.c) By passing various parameters for various timeouts: timeout for
>the initial connection to the server, timeout between chunk
>receptions, timeout of the complete http request
>
>> And no, the opaque type is still as is in the latest and greatest
>version of lwip. Clearly indicating "I shouldn't be using their
>httpc_state_t, right?
>>
>> Would this make sense to you:
>>
>> /** http client tcp recv callback */
>> static err_t
>> httpc_tcp_recv(void *arg, struct altcp_pcb *pcb, struct pbuf *p,
>err_t r)
>> {
>>   httpc_state_t* req = (httpc_state_t*)arg;
>>   LWIP_UNUSED_ARG(r);
>> ...
>> ...
>> ...
>>   if ((p != NULL) && (req->parse_state == HTTPC_PARSE_RX_DATA)) {
>>     req->rx_content_len += p->tot_len;
>>
>>
>> // ----- RESET TIMER TICKS HERE ------
>>     req->timeout_ticks = HTTPC_POLL_TIMEOUT;
>>
>>
>>     if (req->recv_fn != NULL) {
>>       /* directly return here: the connection migth already be
>aborted from the callback! */
>>       return req->recv_fn(req->callback_arg, pcb, p, r);
>>     } else {
>>       altcp_recved(pcb, p->tot_len);
>>       pbuf_free(p);
>>     }
>>   }
>>   return ERR_OK;
>> }
>>
>> This solves my problem, and this way user code is not bothered with
>something I think it shouldn't have to bother with. And the
>httpc_state_t can remain opaque as well.
>>
>> Best regards, bas
>>
>>
>>
>> Op wo 16 jun. 2021 om 14:15 schreef Rémy DZIEMIASZKO
><remy.dziemiaszko@smile.fr>:
>>>
>>> Hello,
>>>
>>> When you call 'httpc_get_file' the parameter 'connection' gives you
>a
>>> handle on the http_state used for that connection
>>> then you can passes this handle to 'httpc_get_file' via the
>parameter
>>> 'void * callback_arg'
>>> then your received function will get the handle via the parameter
>'void * arg'
>>> then in the received function you can do (http_state_t
>>> *)arg->timeout_ticks = MY_RELOAD_VALUE;
>>>
>>> http_state_t * foo;
>>> httpc_get_file(ip, port, uri, settings, my_recv_fn, foo, &foo)
>>>
>>> err_t  my_recv_fn(void * arg, ...)
>>> {
>>>    (http_state_t *)arg->timeout_ticks = MY_RELOAD_VALUE;
>>> }
>>>
>>> You might have a compilation issue because 'http_state_t' is
>normally
>>> an opaque type for the application then the member 'timeout_ticks'
>is
>>> not visible from the application.
>>> In a past project I solved that by exporting the definition of
>>> 'http_state_t' but maybe this is already fixed in the last release
>of
>>> lwip.
>>>
>>> Regards
>>> Rémy
>>>
>>> Le mer. 16 juin 2021 à 13:24, Bas Prins <bas.prins3@gmail.com> a
>écrit :
>>> >
>>> > Dear ,
>>> >
>>> > I don't think I am able to reset the timer. The 'arg' passed  in
>>> >
>>> > err_t rec_fn(void *arg, struct altcp_pcb *conn, struct pbuf *p,
>err_t err)
>>> >
>>> > is not of type httpc_state_t. The rec_fn callback is called here:
>>> >
>>> > static err_t
>>> > httpc_tcp_recv(void *arg, struct altcp_pcb *pcb, struct pbuf *p,
>err_t r)
>>> > ...
>>> > ...
>>> > ...
>>> >     if (req->recv_fn != NULL) {
>>> >       /* directly return here: the connection migth already be
>aborted from the callback! */
>>> >       return req->recv_fn(req->callback_arg, pcb, p, r);
>>> >     }
>>> >
>>> > So what's the deal with this function
>>> >
>>> > /** http client tcp poll callback */
>>> > static err_t
>>> > httpc_tcp_poll(void *arg, struct altcp_pcb *pcb)
>>> > {
>>> >   /* implement timeout */
>>> >   httpc_state_t* req = (httpc_state_t*)arg;
>>> >   LWIP_UNUSED_ARG(pcb);
>>> >   if (req != NULL) {
>>> >     if (req->timeout_ticks) {
>>> >       req->timeout_ticks--;
>>> >     }
>>> >     if (!req->timeout_ticks) {
>>> >       return httpc_close(req, HTTPC_RESULT_ERR_TIMEOUT, 0,
>ERR_OK);
>>> >     }
>>> >   }
>>> >   return ERR_OK;
>>> > }
>>> >
>>> > It only decreases the ticks counter. It never gets reset. So I am
>not allowed to download files longer than 30 secs? If the download
>doesn't succeed within that time, http_client closes the connection.
>>> >
>>> > I would expect that either the timeout counter is reset every
>chunk of data which is received, or that user code (my code) would
>receive a pointer to the httpc_state_t so that it could reset the
>timer.
>>> >
>>> > Can somebody please explain where I am wrong, what am I missing,
>or that indeed the current implementation of http_client is lacking ?
>>> >
>>> > Many thanks in advance
>>> >
>>> > best regards, bas
>>> >
>>> >
>>> >
>>> > Op wo 16 jun. 2021 om 09:29 schreef Bas Prins
><bas.prins3@gmail.com>:
>>> >>
>>> >> Hi all,
>>> >>
>>> >> I found the problem, which was rather easy to find once I
>convinced myself I am man enough to actually read code ;-).
>>> >>
>>> >> But now I wonder how to solve this appropriately. The problem
>seems to be, that this function decreases the ticks, until the timeout
>period is consumed, and then simply closes the connection.
>>> >>
>>> >> /** http client tcp poll callback */
>>> >> static err_t
>>> >> httpc_tcp_poll(void *arg, struct altcp_pcb *pcb)
>>> >> {
>>> >>   /* implement timeout */
>>> >>   httpc_state_t* req = (httpc_state_t*)arg;
>>> >>   LWIP_UNUSED_ARG(pcb);
>>> >>   if (req != NULL) {
>>> >>     if (req->timeout_ticks) {
>>> >>       req->timeout_ticks--;
>>> >>     }
>>> >>     if (!req->timeout_ticks) {
>>> >>       return httpc_close(req, HTTPC_RESULT_ERR_TIMEOUT, 0,
>ERR_OK);
>>> >>     }
>>> >>   }
>>> >>   return ERR_OK;
>>> >> }
>>> >>
>>> >> So I wonder, who should be responsible for resetting the timeout
>counter? My code? The definition of the timeout_ticks seems to only
>visible for to http_client.c.
>>> >>
>>> >> If I simply comment the line which decreases the counter the
>download works.
>>> >>
>>> >> Am I still missing the obvious here?
>>> >>
>>> >> Many thanks.
>>> >>
>>> >> best regards, bas
>>> >>
>>> >> Op ma 14 jun. 2021 om 22:05 schreef Bas Prins
><bas.prins3@gmail.com>:
>>> >>>
>>> >>> Hi Simon, all,
>>> >>>
>>> >>> Fixed the problems I had which led to missing a byte
>occasionally.
>>> >>>
>>> >>> Still unable to download a file using http_client.h. I did learn
>a small part. I log all arguments of
>>> >>>
>>> >>> void result_fn(void *arg, httpc_result_t httpc_result, u32_t
>rx_content_len, u32_t srv_res, err_t err)
>>> >>>
>>> >>> Which produces this log line:
>>> >>> 2021-06-14 21:31:52,041 [DEBUG][thread: 5][UartReader] DOWNLOAD
>finished: httpc_result=5, rx_content_len=5019, srv_res=0, error=0
>>> >>>
>>> >>>   /** Connection timed out (server didn't respond in time) */
>>> >>>   HTTPC_RESULT_ERR_TIMEOUT   = 5,
>>> >>>
>>> >>> I attached the pcap file of tcpdump.
>>> >>> - It contains a couple of attempts to download the file
>(513KB.zip).
>>> >>> - It shows TCP window full warnings. I don't care too much about
>that now. I'll deal with that once I have a stable download going.
>>> >>> - It shows the result of the timeout where my STM board closes
>the connection.
>>> >>>
>>> >>> I also attached my lwipopts.h and the loggings produced by my
>app (including lwip ppp logging).
>>> >>>
>>> >>> Can you help me understand what is going wrong?
>>> >>> Why is LWIP complaining about the server not responding in time,
>while it seems that the server is doing just fine?
>>> >>>
>>> >>> PS: obviously I tested the download on my desktop, I am able to
>download the file repeatedly.
>>> >>>
>>> >>> Many thanks in advance!
>>> >>>
>>> >>> Best regards, Bas
>>> >>>
>>> >>>
>>> >>> Op vr 11 jun. 2021 om 07:15 schreef goldsimon@gmx.de
><goldsimon@gmx.de>:
>>> >>>>
>>> >>>> Am 10.06.2021 um 17:27 schrieb Bas Prins:
>>> >>>> > Hi Simon,
>>> >>>> >
>>> >>>> > Thanks, that's a very likely explanation already. I just
>added the
>>> >>>> > pbuf_free(p) but run into an assert occasionally.
>>> >>>>
>>> >>>> You might have to check for 'p != NULL' just like in tcp recv
>callback
>>> >>>> functions.
>>> >>>>
>>> >>>> Regards,
>>> >>>> Simon
>>> >>>>
>>> >>>> _______________________________________________
>>> >>>> lwip-users mailing list
>>> >>>> lwip-users@nongnu.org
>>> >>>> https://lists.nongnu.org/mailman/listinfo/lwip-users
>>> >
>>> > _______________________________________________
>>> > lwip-users mailing list
>>> > lwip-users@nongnu.org
>>> > https://lists.nongnu.org/mailman/listinfo/lwip-users
>>>
>>> _______________________________________________
>>> lwip-users mailing list
>>> lwip-users@nongnu.org
>>> https://lists.nongnu.org/mailman/listinfo/lwip-users
>>
>> _______________________________________________
>> lwip-users mailing list
>> lwip-users@nongnu.org
>> https://lists.nongnu.org/mailman/listinfo/lwip-users
>
>_______________________________________________
>lwip-users mailing list
>lwip-users@nongnu.org
>https://lists.nongnu.org/mailman/listinfo/lwip-users