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

Include classic Unix.select in the Unix module for Win32 #5329

Closed
vicuna opened this issue Aug 3, 2011 · 6 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Aug 3, 2011

Original bug ID: 5329
Reporter: @dra27
Status: closed (set by @xavierleroy on 2013-08-31T10:44:15Z)
Resolution: fixed
Priority: normal
Severity: feature
Version: 3.12.0
Fixed in version: 3.13.0+dev
Category: ~DO NOT USE (was: OCaml general)
Related to: #5327
Monitored by: @protz @ygrek gildor

Bug description

It would be helpful if the old "classic" sockets implementation of Unix.select were available. This could be called either Unix.old_select, Unix.fast_select, etc.

The old code from select.c can literally be pasted into the present file: the primitive simply needs to be renamed. For compatibility, the Unix distributions of unix.ml would simply need an alias for Unix.select. The documentation need only make clear that on Windows you can only specify socket descriptors.

Additional information

While very useful if you need a fully cross-platform version of select, the present implementation is over-kill (and must be slower, even if not relevantly) if all you're after is select on sockets.

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Oct 23, 2011

Comment author: gerd

Supporting this. Also, the new select has had issues.

In Ocamlnet, the old version of select is available as Netsys_win32.real_select.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 19, 2011

Comment author: @xavierleroy

I thought (but haven't checked) that the Win32 implementation of Unix.select has a fast path for the (common) case where all FDs are sockets. If it's not the case, maybe adding such a fast path without changing the Unix interface would be the best solution. A patch would be welcome.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 19, 2011

Comment author: @dra27

It doesn't appear to - the only call for sockets is to WSAEventSelect. I'm happy to code a patch (it will probably be after Christmas, though - my time has been squeezed recently but I'm working on an OCaml project again in the new year which is affected by this issue so I argue the dedicate time for helping).

My instinct would be to code it so that the default assumption is all sockets: so adapt fdlist_to_fdset to use Descr_kind_val to test each value as it goes and return 0 if they're all sockets and 1 if a non-socket value is found and use that to build either 3 fd_set values or, if any attempt fails, use the new function instead - does that sound reasonable?

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 19, 2011

Comment author: @xavierleroy

The approach you propose sounds fine. Please give it a try when you find the time. Thanks.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 13, 2012

Comment author: @dra27

Patch attached - merges the old unix_select back in as proposed. The patch can be applied with PR5327 (both patches contain an identical fix to windbug.h - so the second one to be applied will fail on that file only). If this patch is applied, the error case in PR5327 can still be seen by adding Unix.stdin to the list of read sockets passed to Unix.select (as that forces the code to use the Sylvain's version of select).

I've not corrected the indentation of the code on lines 1031-1266 as I figured that might cause issues with other pending patches. Those lines should be indented another 4 spaces.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 14, 2012

Comment author: @xavierleroy

Patch applied in SVN trunk (with reindenting of lines 1031-1266).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.