Re: Patch: Improve HTTP time by sending Connection:close

[Daniel Kiper]

On Tue, Nov 15, 2016 at 05:30:28PM +0300, Andrei Borzenkov wrote:
> On Tue, Nov 15, 2016 at 4:21 PM, Daniel Kiper <address@hidden> wrote:
> > On Sat, Nov 12, 2016 at 09:26:17AM -0800, Walter Huf wrote:
> >> GRUB's HTTP module declares support for HTTP/1.1, which defaults to
> >> Connection:keepalive. At the end of the content, the server holds the TCP
> >> connection open waiting for the next request.
> >> It seems that grub_net_poll_cards() is watching for the HTTP module to set
> >
> > Do you have a feeling or are you sure? I think that we have to be sure here.
> >
>
> See http://savannah.gnu.org/bugs/?49531 which has more details.

OK but I think that this should be a part of commit meesage.

> >> net->stall, and otherwise waits the full 400ms GRUB_NET_INTERVAL to return
> >> to processing. However, HTTP module only sets that flag in specific
> >> conditions:
> >>
> >>    - parse_line detects that we are at the end of downloading a chunked
> >>    Transfer-Encoding
> >>    - http_err detects a problem with the underlying TCP connection
> >>    - http_receive has queued 20 netbuffer packets for processing
> >>
> >> If the file is small and takes less than 20 packets, and the server is not
> >> using chunked encoding, grub_net_poll_cards() will wait the full 400ms
> >> before continuing to process and finish the file download.
> >>
> >> This patch sets Connection:close, which will tell the server to close the
> >> connection as soon as it has finished sending the file. GRUB closes any
> >> connections that are left open (in http_seek), so it does not change
> >
> > Why in http_seek? Is it correct or not?
> >
> >> performance. When the server disconnects, I think it triggers http_err and
> >
> > "I think" is too soft. You have to be sure here.
> >
> >> then quits out of grub_net_poll_cards early.
> >
> > Lack of SOB.
> >
>
> I must have missed it. Is it now required?

No (probably it should be sooner or later but we have to discuss this
issue separetly), however, nice to have.

> > Have you tested this patch with large/huge number of files to transfer? I 
> > have
> > a feeling that it can slowdown whole transfer in such cases due to number of
> > connects/disconnects. Maybe this feature should be conditional thing.
> >
>
> Currently grub will always establish new connection in http_open ->
> http_establish, so it should not change anything from grub PoV, but
> reduce amount of lingering connections on server. But I would really
> like to explore another patch in above bug report - gracefully close
> connection when we finished receive data. That said, this one does not

Looking (shortly) at problem description I am not sure that it will fix
the issue. Though I am not convinced that we should play with keep alive
too. Hence, better/nicer solution is appreciated.

Daniel