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

Submitted by:  Walter Huf <snes_rocks>
Submitted on:  Fri 04 Nov 2016 07:41:15 AM UTC  
 
Category: NetworkSeverity: Major
Priority: 5 - NormalItem Group: None
Status: NonePrivacy: Public
Assigned to: NoneOriginator Name: 
Open/Closed: OpenRelease: 
Release: Git masterReproducibility: Every Time
Planned Release: 2.03+

(Jump to the original submission Jump to the original submission)

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

 

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
  • -unavailable- added by arvidjaar (Posted a comment)
  • -unavailable- added by snes_rocks (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 12 Nov 2016 10:14:30 AM UTCarvidjaarPlanned ReleaseNone=>2.03+
    Wed 09 Nov 2016 04:30:32 AM UTCsnes_rocksAttached File-=>Added http-finished.patch, #38911
    Fri 04 Nov 2016 07:41:15 AM UTCsnes_rocksAttached File-=>Added grub-disable-http-keepalive.patch, #38883

    Back to the top


    Powered by Savane 3.1-cleanup1