Fix a bug where downloading files is sometimes very slow

[skaji]

I'm using LWP::UserAgent (and Net::HTTP) for downloading large files.
Then I realized that downloading files sometimes took a lot of time and used a lot of memory.

How to reproduce this issue

I assume you use Linux.

(1) Prepare a 100MB file, and start HTTP server:

❯ dd if=/dev/zero of=file.txt bs=10M count=10

❯ ls -alh file.txt
-rw-rw-r-- 1 skaji skaji 100M Apr 17 01:19 file.txt

❯ plackup -MPlack::App::File -e 'Plack::App::File->new(file => "file.txt")->to_app'
HTTP::Server::PSGI: Accepting connections at http://0:5000/

(2) Prepare the following script, which downloads file.txt to out.txt

# download.pl
use strict;
use warnings;
use LWP::UserAgent;
use HTTP::Request;
my $req = HTTP::Request->new("GET", "http://localhost:5000");
my $res = LWP::UserAgent->new->request($req, "out.txt");

(3) Execute download.pl with time and strace command several times.

❯ time bash -c 'strace perl download.pl 2>&1 | grep read'

Then you will see two cases.
In one case (a), you'll see read(3, ...4096) = 4096 repeatedly, which is expected and takes 6~7sec.

❯ time bash -c 'strace perl download.pl 2>&1 | grep read'
...
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096
...
bash -c 'strace perl download.pl 2>&1 | grep read'  1.87s user 2.58s system 70% cpu 6.310 total

In the other case (b), you'll see read(3, ...1024) = 1024 repeatedly, which is unexpected, takes 20+sec, and uses a lot of memory.

❯ time bash -c 'strace perl download.pl 2>&1 | grep read'
...
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024) = 1024
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024) = 1024
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024) = 1024
...
bash -c 'strace perl download.pl 2>&1 | grep read'  5.22s user 9.71s system 71% cpu 20.975 total

What is the problem?

Net::HTTP::Methods uses my_readline() to read data from socket when it expect "line" data such as HTTP Status line, HTTP Header line, and it keep data in memory.

In current implementation of my_readline(), if $sock->sysread($buf, 1024) returns exactly 1024, then it will retry sysread() again.
I don't see any reason to retry sysread() here, and if we retry sysread(), all data is stored in memory.

Note that we check whether we have "line" data or not at https://github.com/libwww-perl/Net-HTTP/blob/master/lib/Net/HTTP/Methods.pm#L258-L259

How to fix the problem

Do not retry sysread() when $sock->sysread($buf, 1024) == 1024.

See also

Commits related to my_readline()

8311cd1
01f7937

[skaji]

Note that if you prefer as small patch as possible, then the following patch is enough to fix this issue.

diff --git a/lib/Net/HTTP/Methods.pm b/lib/Net/HTTP/Methods.pm
index 7b9aee8..e890b6e 100644
--- a/lib/Net/HTTP/Methods.pm
+++ b/lib/Net/HTTP/Methods.pm
@@ -273,9 +273,7 @@ sub my_readline {
                     my $bytes_read = $self->sysread($_, 1024, length);
                     if(defined $bytes_read) {
                         $new_bytes += $bytes_read;
-                        last if $bytes_read < 1024;
-                        # We got exactly 1024 bytes, so we need to select() to know if there is more data
-                        last unless $self->can_read(0);
+                        last;
                     }
                     elsif($!{EINTR} || $!{EAGAIN} || $!{EWOULDBLOCK}) {
                         redo READ;

[oalders]

This looks good to me.

[skaji]

@oalders Thank you for reviewing this.
I hope this gets merged, and you release a new version of Net::HTTP to CPAN.

[oalders]

@skaji thanks for this. I would just like at least one more person in the org to approve this. @genio did you have a chance to review this?

[genio]

Looks good to me. I also asked for some other eyes and they agreed everything looks good.

[oalders]

I'll probably release a new Net::HTTP on Monday morning.

[skaji]

@oalders @genio thanks!!!

[jonjensen]

I have run into the length modulo 1024 problem in Net::HTTP too. I don't know if it's in this code path or one of the others that have already been fixed, but I would love to see this fix in a released version. Thanks for finding and fixing it!

[oalders]

@jonjensen this was pushed out about a week ago. Thanks for confirming about the bug.

[jonjensen]

@oalders Sorry I missed that fact. Thanks!