sergot / http-useragent

Web user agent class for Perl 6.
MIT License
37 stars 39 forks source link

HTTP::UserAgent fails one exception test on MacOS 10.12.5 #177

Closed cschwenz closed 7 years ago

cschwenz commented 7 years ago
===> Testing: HTTP::UserAgent:ver('1.1.31'):auth('github:sergot')
# NETWORK_TESTING was not set
# NETWORK_TESTING was not set
# NETWORK_TESTING was not set

    # Failed test 'right exception type (HTTP::UserAgent::X::HTTP::Internal)'
    # at /Users/cal/local/share/perl6/precomp/A9FADCBBFA785AF69E3C659EEFF01CB97EFC1545.1498903708.48958/C7/C712FE6969F786C9380D643DF17E85D06868219E line 1
    # Expected: HTTP::UserAgent::X::HTTP::Internal
    # Got:      X::AdHoc
    # Exception message: Could not receive data from socket: Connection reset by peer
    # Looks like you failed 1 test of 3

# Failed test 'throws the correct exception'
# at t/250-issue-144.t line 28
# Looks like you failed 1 test of 1

the perl6 --version is:

This is Rakudo version 2017.06-119-g04746490c built on MoarVM version 2017.06-29-ga51ba620
implementing Perl 6.c.
jonathanstowe commented 7 years ago

That's odd on two fronts, a) that looks like a network test that shouldn't be run without the NETWORK_TESTING and b) it looks like the server end did something unexpected.

Let me have a look.

jonathanstowe commented 7 years ago

Oh right, I'd forgotten what that test actually did. It deliberately checks that behaviour by starting up a server that just closes the connection :-\

I guess the underlying network implementation differs sufficiently on Mac OS that it gives rise to the exception rather than just returning no data.

Unfortunately I'm not in a position to test a fix as I don't have a Mac

cschwenz commented 7 years ago

any suggestions on how to fix?

i'm okay with putting together a pull request, just need some direction on what a correct fix would look like.

:-)

pierre-vigier commented 7 years ago

As a temporary solution, you can install the module with the --force flag,

On mac:

I tried to golf a bit the code, and here is what i found, basically, the test is creating a server that instantly close all the connections, and try to connect to it.

I tried the following, in script1:

#!perl6
use v6;

my $p = start {
    react {
        whenever IO::Socket::Async.listen('localhost', 15260) -> $conn {
            say 'Closing connection';
            $conn.close;
        }
    }

}
sleep 100;

if i run that and in another console, i run a second perl6 script:

#!perl6
use v6;

my $conn = IO::Socket::INET.new(host => 'localhost', port => 15260 );
my $t = $conn.recv( :bin );
say $t.gist;

i can connect to the socket, and i see:

Buf[uint8]:0x<>

However, it i put back the receiving connection within the same script, i have an exception:

Could not connect socket: Connection refused
  in block <unit> at confirm.pl6 line 21

It looks like i can't create the socket and read from it in the same perl6 script

pierre-vigier commented 7 years ago

Also

#!perl6
use v6;

my $p = start {
    react {
        whenever IO::Socket::Async.listen('localhost', 15260) -> $conn {
            say 'Closing connection';
            $conn.close;
        }
    }

}
#sleep 1;

my $conn = IO::Socket::INET.new( host => 'localhost', port => 15260 );
my $t = $conn.recv( :bin );

say $t;

that script triggers an exception if sleep 1 is commented, but not it uncommented, migth be some timing issues as well

ugexe commented 7 years ago
use v6;

my $p = start {
    react {
        whenever IO::Socket::Async.listen('localhost', 15260) -> $conn {
            say 'Closing connection';
            $conn.close;
        }
        whenever IO::Socket::INET.new(host => 'localhost', port => 15260) -> $conn {
            $conn.recv(:bin);
            done();
        }
    }
}

await $p;

@pierre-vigier this should work

pierre-vigier commented 7 years ago

Yes, i agree that would work, but i was trying to golf the problem that is exposed by the test which is broken. However, the exception we receive is Connexion reset by peer, so i'm starting to think that maybe, the behaviour is normal, but should be handle differently by the module, HTTP::UserAgent in that case is expecting a HTTP::UserAgent::X::HTTP::Internal, got an X::AdHoc, however the message looks correct

pierre-vigier commented 7 years ago

In t/250-issue-144.t the server created in the test is directly closing a connection, like following:

my $p = start {
      react {
          whenever IO::Socket::Async.listen('localhost', $port) -> $conn {
            $conn.close;
          }
      }
  }

if i run the test like that on linux it's working, on Mac, i have a connection reset by peer. However, on Mac, using another script to do the rest of the test (basically, running the server in one perl6 command and the test itself in another one), then it's working.

If i add a sleep before closing the connection:

my $p = start {
      react {
          whenever IO::Socket::Async.listen('localhost', $port) -> $conn {
            sleep 1; #like that
            $conn.close;
          }
      }
  }

Then the test is running both on linux and Mac.

If i reduce the time of the sleep, still on my computer, sleep 0.005 is working consistently, less than that and it is sometimes working, sometimes not, which for me would be indicating a race condition somewhere.

jonathanstowe commented 7 years ago

I'm pretty certain that the fix is to catch the "reset by peer" exception in the get-response method and throw the appropriate exception. I just haven't got round to it yet.

jonathanstowe commented 7 years ago

I've just pushed a possible fix to the mac-reset-fix branch is someone on a Mac wants to try that. I'll merge later if it works.

pierre-vigier commented 7 years ago

I had a similar fix in local, and indeed it's working, i just test your branch and it is working for the test case. I guess you can merge.

However, are we not hiding another bug here? How come, on mac the server is returning connection reset by peer if you call it "instantly" but will reply with no data later on. At least, that what IO::Socket::INET is seeing. Anyway, tests are passing without issue with that fix

jonathanstowe commented 7 years ago

If there is a deeper bug it's not in H::UA so we need to work around it. The difference in behaviour might be reportable as a rakudo bug if anyone wants to report it.

I'll merge and close this issue though.