Bug #100654 for IO-Socket-IP: [PATCH] only select for write readiness in connect

Mon Dec 01 22:22:02 2014 craigberry [...] mac.com - Ticket created

Subject:	[PATCH] only select for write readiness in connect
Date:	Mon, 01 Dec 2014 21:21:52 -0600
To:	bug-IO-Socket-IP [...] rt.cpan.org
From:	"Craig A. Berry" <craigberry [...] mac.com>

When doing a non-blocking connect, IO::Socket::IP calls select with a timeout until the connection completes. However, that select call queries for ready-to-read, ready-to-write, and pending error, which doesn't really make sense as select is only documented to respond to a ready-to-write query when a non-blocking connect completes. Changing the call so it only waits for ready-to-write to be set gets t/22timeout.t passing on VMS, where it has failed since inception. The select had been returning immediately without waiting for the timeout. Yes, there is probably something buggy about the VMS implementation that causes it to respond in an incoherent way to an undocumented combination of arguments. But it responds correctly to the documented combination of arguments, and there is no downside to just using the documented combination everywhere. With this change, tests still pass on Windows and OS X as well as starting to pass on VMS. The patch itself is so simple I'll just paste it inline: --- lib/IO/Socket/IP.pm;-0 2014-11-27 08:22:08 -0600 +++ lib/IO/Socket/IP.pm 2014-11-30 20:54:47 -0600 @@ -684,7 +684,7 @@ } my $vec = ''; vec( $vec, $self->fileno, 1 ) = 1; - if( !select( $vec, $vec, $vec, $timeout ) ) { + if( !select( undef, $vec, undef, $timeout ) ) { $! = ETIMEDOUT; return undef; } [end] Possibly easier than applying the patch would be to just edit the first and third arguments to select, replacing them with undef. Show quoted text

________________________________________ Craig A. Berry mailto:craigberry@mac.com "... getting out of a sonnet is much more difficult than getting in." Brad Leithauser

Tue Dec 02 05:26:07 2014 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Mon Dec 01 22:22:02 2014, craigberry@mac.com wrote: Show quoted text

> When doing a non-blocking connect, IO::Socket::IP calls select with a > timeout until the connection completes. However, that select call > queries for ready-to-read, ready-to-write, and pending error, which > doesn't really make sense as select is only documented to respond to a > ready-to-write query when a non-blocking connect completes.

Yes.. It does that at least partly because of MSWin32, which unlike POSIX says, reports only a *successful* connect as ready-to-write. If the connection attempt fails, it only sents pending-error status. Show quoted text

> Changing the call so it only waits for ready-to-write to be set gets > t/22timeout.t passing on VMS, where it has failed since inception. > The select had been returning immediately without waiting for the > timeout. Yes, there is probably something buggy about the VMS > implementation that causes it to respond in an incoherent way to an > undocumented combination of arguments. But it responds correctly to > the documented combination of arguments, and there is no downside to > just using the documented combination everywhere. With this change, > tests still pass on Windows and OS X as well as starting to pass on > VMS.

Interesting that you say it still passes on Windows; I suspect therefore that perhaps some of the tests aren't properly exercising the right behaviours. Did you do that on a real MSWin32, or a Strawberry/Cygwin-related perl? Also, what happens on VMS if you only take out the read-ready, but leave it testing write-ready or pending-error states? -- Paul Evans

Tue Dec 02 05:26:08 2014 The RT System itself - Status changed from 'new' to 'open'

Tue Dec 02 08:28:30 2014 craigberry [...] mac.com - Correspondence added

Subject:	Re: [rt.cpan.org #100654] [PATCH] only select for write readiness in connect
Date:	Tue, 02 Dec 2014 07:28:09 -0600
To:	bug-IO-Socket-IP [...] rt.cpan.org
From:	"Craig A. Berry" <craigberry [...] mac.com>

Show quoted text

> On Dec 2, 2014, at 4:26 AM, Paul Evans via RT <bug-IO-Socket-IP@rt.cpan.org> wrote: > > <URL: https://rt.cpan.org/Ticket/Display.html?id=100654 > > > On Mon Dec 01 22:22:02 2014, craigberry@mac.com wrote:

>> When doing a non-blocking connect, IO::Socket::IP calls select with a >> timeout until the connection completes. However, that select call >> queries for ready-to-read, ready-to-write, and pending error, which >> doesn't really make sense as select is only documented to respond to a >> ready-to-write query when a non-blocking connect completes.

> > Yes.. It does that at least partly because of MSWin32, which unlike POSIX says, reports only a *successful* connect as ready-to-write. If the connection attempt fails, it only sents pending-error status. >

>> Changing the call so it only waits for ready-to-write to be set gets >> t/22timeout.t passing on VMS, where it has failed since inception. >> The select had been returning immediately without waiting for the >> timeout. Yes, there is probably something buggy about the VMS >> implementation that causes it to respond in an incoherent way to an >> undocumented combination of arguments. But it responds correctly to >> the documented combination of arguments, and there is no downside to >> just using the documented combination everywhere. With this change, >> tests still pass on Windows and OS X as well as starting to pass on >> VMS.

> > Interesting that you say it still passes on Windows; I suspect therefore that perhaps some of the tests aren't properly exercising the right behaviours. Did you do that on a real MSWin32, or a Strawberry/Cygwin-related perl?

Windows 7 Pro, recent blead built from scratch using MSVC110 (Visual Studio 2012). But I think you're right that the test isn't really testing what happens when the connect fails while waiting in the select. It might be pretty hard to test that scenario reliably. Show quoted text

> Also, what happens on VMS if you only take out the read-ready, but leave it testing write-ready or pending-error states?

VMS passes the test with only ready-to-read passed as undef and the other two left in, so I'm fine with doing that instead. Show quoted text

________________________________________ Craig A. Berry mailto:craigberry@mac.com "... getting out of a sonnet is much more difficult than getting in." Brad Leithauser

Tue Dec 02 19:09:10 2014 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Tue Dec 02 08:28:30 2014, craigberry@mac.com wrote: Show quoted text

> VMS passes the test with only ready-to-read passed as undef and the > other two left in, so I'm fine with doing that instead.

Just uploaded 0.34 to CPAN, containing this fix. -- Paul Evans

Tue Dec 02 19:09:11 2014 leonerd-cpan [...] leonerd.org.uk - Status changed from 'open' to 'patched'

Fri Dec 05 09:34:02 2014 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Released as 0.34 -- Paul Evans

Fri Dec 05 09:34:03 2014 leonerd-cpan [...] leonerd.org.uk - Status changed from 'patched' to 'resolved'

Fri Dec 05 09:34:03 2014 leonerd-cpan [...] leonerd.org.uk - Fixed in 0.34 added