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

[Rémy DZIEMIASZKO]

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:
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