Re: windows sockets vs. file descriptors bugs in Guile

[Neil Jerram]

Scott McPeak <address@hidden> writes:

>> I've attached my patch for these.  It's a bit simpler than yours, and
>> also avoids needing copyright assignment from you.  Can you take a look,
>> see if you notice any disadvantages compared with your version, and if
>> possible try it out?
>
> Except for the modifications to 'scm_std_select', the two patches are
> nearly equivalent.

Hi Scott,

First, I'm sorry for leaving this hanging for so long.  I'd like to
finalise this, first for 1.8.x and then for master, and the patch that I
now propose for 1.8.x is attached.

0001-Fix-windows-sockets-vs.-file-descriptors-bugs.patch
Description: Text Data

Most of that patch is yours, so we will need a copyright assignment from
you.  Please say if you need help finding and submitting the papers for
that (or if it's a problem).

For master I expect the changes will be different, as I think there is
some support in Gnulib that we can use.  I'll work on that shortly.

In response to your points about the differences between your patch and
a similar one that I had made...

>  The differences are:
>
> * I exported 'scm_is_socket_port()' rather than proliferate the hack
> of checking for equality with 'sym_socket'.  That seemed like a good
> idea because I know the hack is buggy: it is defeated by
> 'set-port-filename!'.

Good point; agreed.

> * I avoided calling socket functions from fports.c, since translation
> units that use socket functions typically have to do a lot of
> gyrations at the beginning to pull in the right headers on the right
> systems.  See the top of socket.c.  Since fports.c does not have all
> that, there is a strong chance that it wouldn't compile on some
> systems, if it were not for the next point.

Again, good point, thanks.

> * I made changes that affect all systems, rather than using #ifdefs
> to make it Windows-only.  As indicated in the comments, there are two
> reasons for this:
>
>   a. Syntax: Code is far less readable when sprinkled with #ifdefs.
>   I've had the displeasure of reading and maintaining code with heavy
>   #ifdefs, and it's not fun.  Guile is currently relatively free of such
>   hacks, because it does a good job of factoring system-specific
>   functionality so it's only exposed to a limited set of modules, and I
>   was trying to keep it that way.
>
>   b. Semantics: Whenever a program explicitly does something different
>   depending on the platform, that's an invitation to a platform-specific
>   bug.  If indeed it is correct to call to 'recv(_,_,_,0)' on a socket
>   instead of 'read(_,_,_)' (an equivalence POSIX guarantees) on Windows,
>   then it should be correct to do so on every platform.  And if it's
>   wrong or inefficient on some platform, we'd like to know ASAP, right?
>
> These differences are mainly stylistic and future-proofing.  You're
> certainly free to ignore them, I just wanted to elaborate on the
> reasons in case they weren't clear.

Those are good arguments, and for master (i.e. current and future
development) I'm happy to accept them (in principle; as already noted, I
expect the exact changes to be different).  For 1.8.x, though, I'd
prefer to make this change in a way that minimises any possible risk to
the existing 1.8.x code, which is already quite well tested on a few
platforms.  Hence in the attached patch there are quite a lot of
#ifdefs; I don't think we need to worry too much about those (and their
maintainability), because this isn't the codebase of the future.

One particular thing is that I didn't like your uses of

  if (SELECT_USE_SLEEP_PIPE)

In fact I'm sure this will cause "conditional expression is constant"
warnings with several compilers.  I've just used more #ifdefs instead.

>>> * scm_std_select: Passes a pipe file descriptor to 'select'.
>>
>> I have no record of hitting this one; I suspect my code at the time just
>> didn't use `sleep'.  I'm wondering if we still need scm_std_select in
>> Guile now anyway.  I'll write again about that.
>
> As for the change to 'scm_std_select', without that change, the code
> does not work.  Using my original testcase (plus a couple of calls to
> 'flush-all-ports', necessary to see the "listening" message, that I
> inadvertently omitted in my original post), Guile-1.8.7 with my
> changes removed and yours added instead says:
>
>   > ./guile.exe -e main -s testsockets.scm server 8888
>   listening on port 8888
>   ERROR: In procedure accept:
>   ERROR: Socket operation on non-socket
>   (exit code of 1)
>
> The reason for this is that 'scm_accept' calls 'scm_std_select', which
> sticks a pipe file descriptor into the set of file descriptors to be
> checked for reading ('readfds'), which Windows does not like.  My
> program never calls 'sleep'--Guile adds the sleep pipe
> unconditionally.

All makes sense; thanks for explaining.  (I can't remember what I had in
mind when I said above that we might not need scm_std_select any more.)

> Once I add just my changes to threads.c on top of your changes to
> socket.c, the test case works perfectly.
>
> While the changes to 'scm_std_select' are certainly the ugliest of
> those I proposed, some sort of change is required in order to get
> socket server processes to work in Guile-1.8.7 on Windows, as my test
> case shows.

Yes.  I added a note in the patch about the consequence of not
implementing the sleep pipe - and the upshot is that I agree with you
that this is a useful compromise.  Please let me know if you have any
comments on that.

Regards,
        Neil