GNU 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: |
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.
|
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)
This changes logic without any explanation why.
+ if (data->size_recv && data->downloaded_size >= file->size)
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>![]() |
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).
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>![]() |
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.
|
Walter Huf <snes_rocks> |
Depends on the following items: None found
Items that depend on this one: None found
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 Release | None | ![]() |
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 |
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?