Re: ws2_32: WSACleanup cleans up open sockets

[Ken Thomases]

On Aug 25, 2015, at 2:18 PM, Matt Durgavich  wrote:
> @@ -4987,6 +5023,11 @@ static int WS2_sendto( SOCKET s, LPWSABUF lpBuffers, DWORD dwBufferCount,
>      DWORD bytes_sent;
>      BOOL is_blocking;
> - if (!socket_in_table(s)) {
> -
>    SetLastError(WSAENOTSOCK);
> -
>    return SOCKET_ERROR;;
> - }
> -

I raised this in my reply to Henri, but: why was this added?  And if it's sensible to have it here, why not in any of the other functions?
> +static void cache_update(SOCKET new_entry, SOCKET old_entry)

Still calling it a "cache"?
> +{
> - int index, depth, tries;
> - LONG *bucket;
> -
> - index = socket_to_index(new_entry == 0 ? old_entry : new_entry);
> - depth = 0;
> - bucket = (LONG*)&socket_table[index];
> - tries = 0;
> - do
> - {
> -
>    LONG *slot = (LONG*)&bucket[depth++];
> -
>    if (InterlockedCompareExchange(slot, new_entry, old_entry) == old_entry)

I think it would be better to do:

if (*slot == old_entry && InterlockedCompareExchange(slot, new_entry, old_entry) == old_entry)

While InterlockedCompareExchange() is faster than locking, it's still not cheap.  Doing it on every element without the prior belief that that element matches seems wasteful.
> -
>        break;
> -
>    if (depth == BUCKET_DEPTH)
> -
>    {
> -
>        ++tries;
> -
>        depth = 0;
> -
>    }
> -
> - } while (tries < 3);
> - if (tries == 3)
> - {
> -
>    ERR("Socket hash table bucket overflow. Resize buckets");
> - }
>
> +}

I don't see any reason to try multiple times.  The chances of a slot changing from one pass to the next is miniscule.  If you're searching for a free slot (old_entry is 0) and you don't find one on the first pass, then you should still resize the buckets, even if you would hypothetically find one on a second or third pass.  If old_entry is not 0, then it's really unlikely that another thread would add that specific socket to the table.  How could that happen that wouldn't be a bug?
> -
>
> +static void add_to_table(SOCKET s)
> +{
> - cache_update(s, 0);
>
> +}
> -
>
> +static BOOL remove_from_table(SOCKET s)
> +{
> - if (!socket_in_table(s))
> -
>    return FALSE;
> -
> - cache_update(0, s);
> - return TRUE;

This is two passes when one should do.  More importantly, though, it seems race-prone.  If two threads are both closing the same socket, they could each first determine that the socket is in the table and thus return TRUE, when only one should return TRUE.

You could make cache_udpate() return a success/failure value and simply make this a wrapper around cache_update(0, s).

-Ken