[En-Nut-Discussion] Http infinite loop problem.

[Thiago A. Corrêa]

This is a follow up with a discussion I was having with Ole in Re:
[En-Nut-Discussion] Duplicate code in httpd.c and ssi.c

Ole, devs,

   I think I figured out what is wrong with the http code.
   The addition of Keep-Alive http connections have brought to light
an unexpected issue. The code now works inside a loop to service
different requests from the browser. The problem is, that it uses
fgets to read the next request inside that loop, and fgets never times
out and blocks if there is nothing available to read.
   I was able to get the code to work back again by adding a timeout
to the socket before passing on the stream to NutHttpProcessRequest,
but then it always takes a timeout to finish loading cgi's and asp's.
Not to mention that it requires code change on existing applications.

  I don't know if there are any good solutions to this problem. One
easy hack would be set req->req_connection = HTTP_CONN_CLOSE; when we
process asp's and cgi's. I don't know if this is acceptable or not.

  I have no idea why it happens only to asp and cgi though. It might
be that's just because it's the last thing that the browser requests
on my use case.

Kind Regards,
    Thiago A. Correa

On Wed, May 14, 2008 at 3:40 PM, Thiago A. Corrêa
<thiago.correa at gmail.com> wrote:
> On Wed, May 14, 2008 at 5:16 AM, Ole Reinhardt
> <ole.reinhardt at embedded-it.de> wrote:
>> Hi Thiago,
>>
>>>    Actually it's probably non-related. I still have to figure out
>>> what's wrong, if it was the ethernut upgrade or something else. I have
>>> an index.html that redirects to index.asp (my code is older than the
>>> patch that introduces index.asp as a valid default page).
>>>    So, I get to load index.html but not index.asp. Typing the cgi urls
>>> that I have directly also doesn't help. The browser just sits there
>>> "loading" indefinitly.
>>
>> Strange, try to trace your network traffic with wireshark to see at what
>> point the communication stops. Could be a stack overflow anywhere in
>> your code?
>
> I don't think so, as I have reverted my code to previous working
> revisions. Plus, other threads keep working while the HTTP connection
> seams to be stuck. I have just run a Wireshark session on the HTTP
> traffic, and now I'm quite convinced that the recent changes have
> smoething to do to it. I see that the HTML output is right for my
> index.asp, but there is no connection closing, no FIN.
>
> I guess the next step is revert those changes in my local copy and
> recompile ethernut to see if there is a difference. I have inspected
> the code, and it sounds solid though. If this change is indeed the
> problem, more eye balls there would be quite helpfull.
>
>> Right. We should avoid malloc / free and use NutHeapAlloc / NutHeapFree
>> instead. That would be more consistent.
>
> Is there an unspoken rule of the thumb on using NutHeapAlloc and
> malloc? Is malloc supposed to be used only on client code?
>
>>> My proposal isn't really to fix a problem, but just rather get rid of
>>> a duplicate function. Should reduce code size a little bit. Anyway, I
>>> produced a patch that I'm attaching to this mail. Will commit if
>>> everything is in order.
>>
>> I'm wondering why you remove the DestroyRequestInfo in both files?
>> Btw: Those two versions of DestroyRequestInfo are different! The one in
>> httpd.c frees one more member of the request struct.
>>
>> So remove the right one :)
>
> It seams that none of the files is the right one to keep it :)
> So, I created an httpd_p.c/h. This is borrowed from my experience
> hacking Qt internals. They use the _p suffix for files that are
> "private", containing functions that are not part of the public API.
> It makes a lot of sense to me, so I thought I could borrow the concept
> here.
> The DestroyRequestInfo in httpd.c seams more complete. It checks for
> more fields, and checks for bad parameter.
>
>> Perhaps the best way would be to place some common functions into a
>> httpd_commom.c. There are some more functions I found that are used in
>> ssi.c and are included from httpd.c. Cross referencing functions between
>> c files is not a realy good coding style :)
>
> That's what the linker is good for :)
> Since I've introduced a private header that declares the function, it
> seams ok. I thought for a second about forward declaring the function
> in the c file directly, that would be quite bad :)
> Doesn't seam much different than dencode.c/h, except that I added
> comments explicitly warning that those are not part of the public API
> and are subject to change or removal at any time.
>
> Those other functions are static or part of the public API? I think we
> could place all those shared functions that are not part of the API
> there, and reduce the code size of the lib and increase
> maintainability as a bonus.
>
> Regards,
>    Thiago A. Correa
>