AnyEvent and Strawberry perl issue

[Steve Hay]

On 14 March 2015 at 23:37, Marc Lehmann <schmorp at schmorp.de> wrote:
> On Sat, Mar 14, 2015 at 02:49:34PM +0000, Steve Hay <steve.m.hay at googlemail.com> wrote:

>> >             (connect $state{fh}, $sockaddr)
>> >             || ($! == Errno::EINPROGRESS # POSIX
>> >                 || $! == Errno::EWOULDBLOCK
>> >                 # WSAEINPROGRESS intentionally not checked - it means something else entirely
>> >                 || $! == AnyEvent::Util::WSAEINVAL # not convinced, but doesn't hurt
>> >                 || $! == AnyEvent::Util::WSAEWOULDBLOCK)
>> >
>> > cygwin gets this right as well.
>> >
>> > So this change probably breaks connections because the WSA error code is
>> > mapped on the wrong POSIX code. I don't think this can be fixed nicely
>> > either, it needs to be fixed in perl, where the incorrect mapping is.
>>
>> I do not see how this will break.
>
> Of course not, that would have required some research and diligence. Or
> maybe skimming my original mail, which explains it. Too difficult for you?
>
> I keep repeating myself, but here, again, in slow motion, just for you
> who has so much trouble reading (or, as I have to suspect, ignores valid
> arguments on purpose):
>
> 1. WSAEINPROGRESS has nothing whatsoever to do with EINPROGRESS. perl
>    merged both to mean the same.
> 2. if connect returns with an error of WSAEINPROGRESS, it means the connect
>    has failed.
> 3. if connect returns with an error of EINPROGRESS on a unix (or POSIX
>    platform), it means the connect is under way, and the user will
>    be notified.
> 4. if connect returns with an error of EINPROGRESS on some perl win32
>    configs, it means the connect has failed, and no notification will
>    ever arrive.
>
> Code that relies on the correct error codes will timeout (at best) or
> hang due to this change, instead of returning an error (because it will
> misinterpret WSAEINPROGRESS as EINPROGRESS, which has completely different
> meaning).
>
> AFAICS, this is true for every single case of WSAEINPROGRESS - it always
> means that the operation has not been done, while the POSIX EINPROGRESS
> code means the opposite.
>
>> So in all cases the test against EWOULDBLOCK works and there is no
>> breakage.
>
> Obviously wrong, as was clear from the beginning. Do you see it now and
> are you honest enough to admit to have fucked up?
>

Thank you for explaining this again. I'm sorry if you feel that my
previous reply was intended as a smokescreen of any kind. I can only
assure you that it wasn't meant that way. I was simply responding to
points that you raised as I understood them. I do not think it would
be productive to reply to every point one by one again now, so I will
just reply to the main issue that I overlooked before:

I did gloss over the (WSA)EINPROGRESS issue, wrongly believing that
the check against EWOULDBLOCK would save the day in this case given
that WSAEWOULDBLOCK is now mapped to EWOULDBLOCK and you had noted
that "AnyEvent::Socket relies on the correct error code
(WSAEWOULDBLOCK instead of EINPROGRESS)".

Looking again, I can see how the code above is a problem when
connect() fails on Windows and WSAGetLastError() returns
WSAEINPROGRESS (10036). That return code is mapped to EINPROGRESS
(112) before assignment to $!, and Errno::EINPROGRESS is also 112, so
the test for $! == Errno::EINPROGRESS (which on *nix means the connect
is under way) succeeds and the Windows code wrongly enters the
"success" code when in fact connect() has failed.

However, it is not the recent changes in question (i.e. those merged
in by commit ea95436966) that have brought about this particular
failure mode. Prior to those changes (i.e. in 5.19.3 and earlier) it
is true that the WSAGetLastError() code (10036) would have been
assigned directly to $! but the value of Errno::EINPROGRESS in
pre-5.19.3 perls was also 10036 so the (*nix-oriented) test for
whether $! == Errno::EINPROGRESS would still have wrongly succeeded
then anyway, and the Windows code would still have wrongly entered the
"success" code when connect() has failed.

This has been the case for a very long time now, e.g. I just checked
on 5.8.1 and Errno::EINPROGRESS is also 10036 there, matching the
value assigned to $! on such a connect() failure.

I am not saying that the WSAExxx / Exxx mapping is correct (indeed, it
looks positively incorrect in the light of many examples that you've
now given), but it had already been in place in some form for many,
many years with no problems reported that I was aware of, so I did not
think to question it when I added more of the same recently.

I agree that it would be good to fix this if possible, although given
that the origins of the WSAExxx / Exxx mapping are so old I would not
see this as a showstopper for 5.22 (due in May).