Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace EGAIN with EWOULDBLOCK - these are same on UNIX but on Windows y... #11

Closed
wants to merge 52 commits into from
Closed

Conversation

noxxi
Copy link

@noxxi noxxi commented Jan 9, 2015

Since 2.006 IO::Socket::SSL supports non-blocking on Windows. But on Windows the relevant $ERRNO is EWOULDBLOCK, not EAGAIN. On Unix these both are the same. Thus the code needs be changed to use EWOULDBLOCK instead.

This patch needs to go together with libwww-perl/libwww-perl#66

See also http://stackoverflow.com/a/27863737/3081018

@frioux
Copy link
Member

frioux commented Jan 26, 2015

For what it's worth we've run into this and this patch solves the problem

@frioux
Copy link
Member

frioux commented Jan 26, 2015

I think that the change will need to be applied to https://github.com/libwww-perl/libwww-perl/blob/master/lib/LWP/Protocol/http.pm too.

@noxxi
Copy link
Author

noxxi commented Jan 26, 2015

I think that the change will need to be applied to https://github.com/libwww-perl/libwww-perl/blob/master/lib/LWP/Protocol/http.pm too.

Yes, this is done in libwww-perl/libwww-perl#66

@ilmari
Copy link
Contributor

ilmari commented Jan 27, 2015

To be properly correct, you'd have to check for both EAGAIN and EWOULDBLOCK, since they're not required to be the same according to POSIX, and read() and write() can use either to signal no available data or buffers full, respectively, on a non-blocking socket.

@noxxi
Copy link
Author

noxxi commented Jan 27, 2015

To be properly correct, you'd have to check for both EAGAIN and EWOULDBLOCK ...

You are right. Adapted the fix accordingly.

@Zarabozo
Copy link

Zarabozo commented Feb 9, 2015

Wow. This issue was driving me crazy! I had to patch both Net/HTTP/Methods.pm and LWP/Protocol/HTTP.pm. The only workaround without patching them is to use IO::Socket::SSL version 2.005, which is the last version still not supporting non-blocking sockets on Windows.

@karenetheridge
Copy link
Member

I'm uncomfortable merging this without more voices in agreement - e.g. there was just a big discussion on the perl5porters list about EAGAIN/EWOULDBLOCK on various platforms (and e.g. see https://api.metacpan.org/source/MLEHMANN/AnyEvent-7.09/Changes).

@noxxi
Copy link
Author

noxxi commented May 8, 2015

As far as I can see from the Changes and the discussion the problems described affect EWOULDBLOCK only if $! was explicitly checked against the WSAEWOULDBLOCK value of 10035. It does not affect any code which just checks against the EWOULDBLOCK constant because both $! and the constant where changed to the same value. The major problem described in the discussion is EINPROGRESS and the similar named but semantically different WSA constant. But this problem does not affect this issue here but only asynchronous connects. Thus I don't think that the suggested fix for the problem does any harm, but it instead fixes the problem.

@genio
Copy link
Member

genio commented Jan 9, 2017

@noxxi Can you rebase this please?

@karenetheridge
Copy link
Member

That's not a rebase.

git checkout nonblock-fix
git rebase master
<fix any conflicts that arise; the -i option may be handy for the rebase operation>
git push --force

karenetheridge and others added 26 commits February 19, 2017 08:55
…se_size % 1024 == 0

my_readline() is broken for response sizes (headers + raw body) that are multiples of 1024 (sysread block size).

When reading from date from the socket, we keep reading until we get fewer bytes than we asked for. The code implicitly assumes that if we're getting *exactly* what we asked for, then there is more data available. That assumption does not hold true for response sizes of 1024, 2048, etc...

In most cases, this bug is masked by either not using keep-alive or by a lot of servers having short keep-alive timeouts. It is easily reproducible though by using a simple Plack one-liner and tweaking the response size until hitting e.g. 1024 bytes in the first sysread(). The following works with starman:

	$ cat foo.psgi
	sub { [ 200, [], [ "x" x 904 ] ] } # Tweak response size as needed, depending on headers sent by starman
	$
	$ starman --keepalive-timeout 10 foo.psgi

Elsewhere, run:

	$ time perl -MLWP::UserAgent -e 'print LWP::UserAgent->new(keep_alive => 1)->post(q(http://127.0.0.1:5000))->status_line'
	200 OK
	real	0m10.067s
	user	0m0.052s
	sys	0m0.004s
	$

Net::HTTP hangs, waiting for more data until the server disconnects.

The simplest fix seems to be just checking can_read() before another read if we get exactly what we asked for in sysread().
…CE at apache.org. Apache recently changed their server config and no longer allow TRACE requests.
Test against http://httpbin.org/headers and iterate until
responses are exactly 1024, 2048 or 3072 bytes in size.
    - Updated the Changes file
    - When using Net::SSL, pending data was potentially ignored GH PR#7 (Jean-Louis Martineau)
    - Fix prereqs
@noxxi
Copy link
Author

noxxi commented Feb 19, 2017

Something got messed up while trying to rebase. I'll restart with a new repo because I cannot sort this out. Continued in #24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet