bugGNU GRUB - Bugs: bug #49531, HTTP module does not support HTTP...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #49531: HTTP module does not support HTTP Pipelining

Submitter:  Walter Huf <snes_rocks>
Submitted:  Fri 04 Nov 2016 07:41:15 AM UTC
   
 
Category:  Network Severity:  Major
Priority:  5 - Normal Item Group:  None
Status:  None Privacy:  Public
Assigned to:  None Originator Name: 
Open/Closed:  Open Release:  Git master
Release:  Reproducibility:  Every Time
Planned Release:  2.03+

Jump to the original submission

Mon 22 Apr 2019 03:28:14 AM UTC, comment #8: 

I'm trying to debug some connection timeout issues and came across this bug. At least the Connection: close part of it seems trivial and uncontroversial -- perhaps it could be merged at this point?

Faidon Liambotis <paravoid>
Sat 12 Nov 2016 06:04:42 PM UTC, comment #7: 

I moved the grub_net_tcp_stall call to require 100 packets instead of 20, to match the earlier block.

data->size_recv is the flag that specifies whether it knows the full size of the file, which is why it's in the if statement.

parse_line only sets stall if it reaches the end of the chunk, I think; I'm not quite sure what the various in_chunk_len states mean.
parse_line is not triggered for any data handling after the headers if it is not in Chunked Encoding mode. I can't seem to force Nginx to send static files in Chunked Encoding mode, which would otherwise work around this issue.

Walter Huf <snes_rocks>
Sat 12 Nov 2016 10:14:31 AM UTC, comment #6: 

I think at this point it is better to move discussion to grub-devel, savannah bug tracker is not really suitable for patch review. Some comments regarding second patch.

    if (file->device->net->packs.count >= 20)
-     {
-       file->device->net->stall = 1;
+     file->device->net->stall = 1;
+
+   if (file->device->net->packs.count >= 100)
        grub_net_tcp_stall (data->sock);
-     }

This changes logic without any explanation why.

+   if (data->size_recv && data->downloaded_size >= file->size)
+     file->device->net->stall = 1;

file size may also be unknown in advance. We also already have code in parse_line() to set stall and eof. Why is it not kicked in?

As for the original problem, looks OK but I would prefer to delay it for post 2.02; it is not critical and such changes always have potential for regressions. Thank you for understanding.

Andrei Borzenkov <arvidjaar>
Group Member
Wed 09 Nov 2016 04:31:26 AM UTC, comment #5: 

I just added a patch that allows the HTTP driver to notice when it has finished downloading a file and jump out of grub_net_poll_cards early.

Walter Huf <snes_rocks>
Mon 07 Nov 2016 02:41:02 AM UTC, comment #4: 

I have figured out where the delay comes from, and it's actually 400ms (GRUB_NET_INTERVAL).
In grub_net_fs_read_real (http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/net/net.c#n1583), it checks to see if any packets are ready to process, and then runs grub_net_poll_cards. I think this causes the TCP and HTTP stacks to process packets until HTTP decides it has enough packets (20 packets, according to http.c:277) and sets net->stall, which causes grub_net_poll_cards to quit out before the time is finished.
However, most reads (fetching a module, reading menu.lst) are smaller than 20 packets, and so this poll lasts the entire GRUB_NET_INTERVAL of 400ms. When this finishes, it then continues parsing packs and returns when len==0.

Altering the timeout to this grub_net_poll_cards causes a change in the delay I see in the packet trace.

When the server disconnects, I think it triggers http_err, which sets net->stall==1 and causes grub_net_poll_cards to finish up early.

Walter Huf <snes_rocks>
Mon 07 Nov 2016 12:02:37 AM UTC, comment #3: 

I am not sure where the 300ms delay comes from. Wireshark shows that all the data is sent by the server, all the acks are sent by the client, and then the client sends a FIN-ACK sometime 300ms later. I am able to reliably reproduce it, should I upload a packet trace? How can I add debugging to help trace it?

HTTP/1.1 adds support for the Range and Host headers, and the code already implements the chunked Transfer-Encoding that was added in HTTP/1.1.

Walter Huf <snes_rocks>
Sun 06 Nov 2016 06:45:08 AM UTC, comment #2: 

Can you explain in more details where 300ms delay comes from?

Do we use any HTTP 1.1 feature in the first place? Announcing HTTP/1.0 looks more simple.

Andrei Borzenkov <arvidjaar>
Group Member
Sun 06 Nov 2016 01:18:27 AM UTC, comment #1: 

Correction, it declares HTTP/1.1, which defaults to Connection:keepalive.

Walter Huf <snes_rocks>
Fri 04 Nov 2016 07:41:15 AM UTC, original submission:  

The HTTP module declares support for HTTP/1.0, which defaults to Connection:keepalive. However, Grub's HTTP client doesn't seem to properly support pipelining, and waits for 300ms at the end of a request before closing the connection and opening a new connection for the next request.
The proper fix would be to notice when the Content-Length number of bytes have arrived and then keep the connection around for the next request.
An easier solution is to have the client declare Connection:close, which will make the server close the connection at the end of the file and allows Grub to fetch the next file much faster.

Walter Huf <snes_rocks>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #38911:  http-finished.patch added by snes_rocks (2KiB - application/octet-stream - A patch that stalls the HTTP connection when a file has finished downloading)
file #38883:  grub-disable-http-keepalive.patch added by snes_rocks (1KiB - application/octet-stream - A patch that disables HTTP Pipelining)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by paravoid (Posted a comment)
  • -email is unavailable- added by arvidjaar (Posted a comment)
  • -email is unavailable- added by snes_rocks (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-11-12 arvidjaar Planned ReleaseNone 2.03+
    2016-11-09 snes_rocks Attached File- Added http-finished.patch, #38911
    2016-11-04 snes_rocks Attached File- Added grub-disable-http-keepalive.patch, #38883

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code