Tcl Source Code

This patch add the autoconf detection and C code necessary to use poll() instead of select() in TclUnixWaitForFile. Since this function only waits on one file descriptor, using poll() allows for slightly smaller code, less variables and a little less complexity.
The code which uses select() has been altered to use the FD_SET macros in a similar manner as to what was done to tclUnixNotfy.c in this update: http://tcl.cvs.sourceforge.net/tcl/tcl/unix/tclUnixNotfy.c?r1=1.11.2.4&r2=1.11.2.5

Please scrutinize.

committed FD_* macros cleanup to HEAD and core-8-5-branch

closing as rejected w.r.t the poll proposal

FWIW, the changes to the macosx notifier I mentioned here are now complete and at
http://github.com/das/tcltk/blob/083fb1d55ab969960e9d18fa21f3ac5043432708/tcl/macosx/tclMacOSXNotify.c
This includes the unification of TclUnixWaitForFile() and Tcl_Sleep() with the notifier as discussed here
I have indeed not had time yet to finish my port of these changes to the unix notifier (and may not for some time).

I have no problem with the select() cleanup part of your patch and am willing to apply those bits, but continue to be opposed to adding a dependency on poll for all the reasons previously mentioned here.

assigning to me and raising prio for the select bits

Returning to this a little early (I was thinking one year of inactivity), I see that the FD_SET stuff hasn't even made it in, never mind the various cleanup/redo phantom proposals that have yet to materialize.

Logged In: YES 
user_id=68433
Originator: NO

See also #1994512.

Logged In: YES 
user_id=90580
Originator: NO

Stuart,
IMO the main difficulty with this patch is that it adds a dependency on poll(), which we have to detect and make sure is useable on all supported platforms, for instance on Darwin, poll() is known to be buggy in certain OS versions and with certain fd types (e.g. with devices in OSX 10.3 and earlier, c.f. google for more), it is very possible similar problems exist on other platforms.
I'm not convinced it is worth the time & effort to do this if the whole approach to call select() or poll() in TclUnixWaitForFile() directly is flawed anyway...

Logged In: YES 
user_id=143350
Originator: YES

Sounds good - I await your patch. ;)

In the meantime ...

This does not 'attack the wrong problem', it is a simple patch designed to make a couple of simple improvements, maybe save a bit of mem & time, as well as providing me with the valuable experience.

Logged In: YES 
user_id=90580
Originator: NO

Stuart,
my point is that rather than integrate the adjustments made to tclUnixNotfy.c (which I agree should have been made here at the same time),  TclUnixWaitForFile() should instead directly use the notifier machinery in tclUnixNotfy.c to wait for its one fd, the code in Tcl_WaitForEvent() already knows how to do that...

Logged In: YES 
user_id=143350
Originator: YES

The purpose of this patch is to replace select() with poll() for the case of one fd, leaving in both implementations for purposes of testing and evaluation.

One thing not to overlook please (unless it's already been done or doesn't matter) are the adjustments to the code using select() to match those done here:

http://tcl.cvs.sourceforge.net/tcl/tcl/unix/tclUnixNotfy.c?r1=1.11.2.4&r2=1.11.2.5

Logged In: YES 
user_id=90580
Originator: NO

IMO the fact that TclUnixWaitForFile() and Tcl_Sleep() call select() directly is fundamentally wrong anyway, they should be using the same mechanism as the notifier to suspend, i.e. in the threaded case the notifier thread should be doing the select for TclUnixWaitForFile(), same as waiting/polling from Tcl_WaitForEvent(). Calling select() directly prevents being woken up from another thread (except through a signal), which e.g. prevents being canceled via TIP285 during a waitforfile or sleep.

Moreover, as I discovered in my current reworking of the Mac OS X CoreFoundation notifier to add support for embedding, the notifier not being used in TclUnixWaitForFile() and Tcl_Sleep() forces an alternative event loop to #ifdef out the standard unix versions of these and replace them (in my case with versions based on CFRunLoop to ensure the embedding event loop keeps processing events during waitforfile/sleep).
If an embedder wants to replace the notifier via Tcl_SetNotifier() this workaround is not possible, and as TclUnixWaitForFile()/Tcl_Sleep() are not hookable, they will very likely be a big problem for any custom event loop (such as the Xt notifier)...

This would all go away if the low-level waiting action in the notifier were factored out (i.e. signaling the notifier thread and waiting on the condition variable resp. calling select() in the single-threaded case), TclUnixWaitForFile() and Tcl_Sleep() could then call this new low-level wait instead of select() directly.

I intend to work on doing exactly this in the next few weeks, as part of my look at the unix notifier to delay notifier thread creation until needed, so no need to waste further cycles looking at this patch, it attacks the wrong problem IMO.

Logged In: YES 
user_id=496139
Originator: NO

Joe, if I read you correctly:

> Anyway, this routine already has plenty of unnecessary complexity as it
> is; adding an #ifdef HAVE_POLL branch would just double the amount.

... it seems that we agree ;-)
I propose to Close/Reject.
Better to concentrate our energy on cleaning such things up rather than letting them proliferate.
Donal, your advice ?

Logged In: YES 
user_id=68433
Originator: NO

Re: poll vs. select: It used to be the case that more systems had select() than had poll(), but that was years ago.  Nowadays -- like, the last 10 or 15 years -- you can count on both being available.  You have to go back pretty far to find a Unix that doesn't have poll() (BSD 4.3 and SunOS 3 are the latest examples I can find.)

Neither poll() nor select() were included in POSIX.2 (circa 1993), although they were available in Unices of that era.  The Open Group's Unix95 standard (1995) mandated both, and they've been part of the SUS ever since then.

Logged In: YES 
user_id=68433
Originator: NO

(sorry, last comment was from me, I forgot to log in).

Also: TclUnixWaitForFile() is also only called with mask = (TCL_WRITABLE | TCL_EXCEPTION).

Anyway, this routine already has plenty of unnecessary complexity as it is; adding an #ifdef HAVE_POLL branch would just double the amount.

Logged In: NO 

Some notes:

(1) TclUnixWaitForFile() is only used in one place in the
core proper, namely WaitForConnect().

(2) In WaitForConnect(), the "timeout" parameter is always either
'0' (do not wait) or -1 (wait forever).  So TclUnixWaitForFile()
is much more complicated than it needs to be -- it doesn't need the
"mini-event loop" or all the timeout processing at all.

(3) WaitForConnect() and the TCP_ASYNC_SOCKET/TCP_ASYNC_CONNECT
logic is also more complicated than it needs to be (and might
not be necessary at all).

(4) TclUnixWaitForFile() is also used in the test suite, this time
with timeout values > 0.  Whether the test suite is calling it simply
to test TclUnixWaitForFile() itself or if it's needed to test
something "real", I was not able to determine.

(5) TclUnixWaitForFile also appears in the internal stubs table.
Whether there are any callers outside of the core is difficult
to determine.

Logged In: YES 
user_id=79902
Originator: NO

I've poked around in the guts of select() and poll() on a few systems (long ago; many details forgotten) and one thing I remember is that on some systems you have poll() being used to implement select(), and on others it is the other way round!

Both are common though; might even be that both are POSIX...

Logged In: YES 
user_id=496139
Originator: NO

While I agree with the idea that the poll() API goes in the right direction, I'm wondering: this introduces a branch, two families of implementations depending on HAVE_POLL. Any idea of how many systems have and don't have poll today, and how many years the situation is expected to last, with the two branches to maintain ?

File Added - 277259: tcl_poll.patch