Bug #17394: TCPServer is not thread safe on win32

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 2.7.1
• Backport: 2.6: REQUIRED, 2.7: DONE, 3.0: DONE

[puchuu (Andrew Aladjev)]

Hello, it looks like win32 version of TCPServer is not thread safe. I've extracted the following code from project:

require "socket"
require "parallel"

::Parallel.each(1..2, :in_threads => 2) do
  ::TCPServer.open 0 do |server|
    thread = ::Thread.new { server.accept.close }
    ::TCPSocket.open "localhost", server.addr[1]
    thread.join
  end
end

It returns:

in `accept': Bad file descriptor - not a socket file descriptor (Errno::EBADF)

You can fix this code by using :in_threads => 1. This is unique windows issue: GNU/Linux, FreeBSD and OSX works perfect. I think something is wrong in win32 folder of ruby source code.

Can you please assign this issue to the developer familiar with win32 folder? Thank you.

[puchuu (Andrew Aladjev)]

PS This issue is not 100% reproducible, please try to run script several times, thank you.

[puchuu (Andrew Aladjev)]

• File Screenshot_20201215_020316.png Screenshot_20201215_020316.png added

I've added a screenshot from windows virtual machine on mingw64 env.

[puchuu (Andrew Aladjev)]

The problem is socklist_insert function, you can just add the following code:

socklist_insert(r, 0);
fprintf(stderr, "fit %d\n", is_socket(TO_SOCKET(fd)));

is_socket has a chance to be false. is_socket is just an alias to socklist_lookup. So we can see that socklist_insert and socklist_lookup somehow used in wrong way.

[puchuu (Andrew Aladjev)]

You can easily reproduce this issue by using the following code:

// socklist_insert(r, 0);
st_data_t data;

int insert_result_0 = st_insert(socklist, (st_data_t)r, (st_data_t)0);
int lookup_result_0 = st_lookup(socklist, (st_data_t)r, &data);
fprintf(stderr, "fit 0 %d %d\n", insert_result_0, lookup_result_0);

int insert_result_1 = st_insert(socklist, (st_data_t)r, (st_data_t)0);
int lookup_result_1 = st_lookup(socklist, (st_data_t)r, &data);
fprintf(stderr, "fit 1 %d %d\n", insert_result_1, lookup_result_1);

It is possible to receive "fit 0 0 0" and "fit 1 1 1" on windows, it means that first st_insert silently failed without error code, but second attempt succeed.

[puchuu (Andrew Aladjev)]

I've found the source commit 068dd60372d659c97a76224dfce38ed96984cb6f where socklist was introduced. st_table is just hash table, it looks like original code provides global socklist hash table and writes into it without any locks. This code was designed for single thread mode only.

Today we can see that multiple GetCurrentThreadId is processing single global socklist, we need to add locks.

[puchuu (Andrew Aladjev)]

Added fix https://github.com/ruby/ruby/pull/4212

[jeremyevans0 (Jeremy Evans)]

• Status changed from Open to Closed

Fix merged at 0d76636117c99921ac7c43293ba7962d22e72fbd .

[puchuu (Andrew Aladjev)]

• File 4220.patch 4220.patch added

I've prepared backports for ruby_3_0 and ruby_2_7 on github, but ruby_2_6 is not using github, so i am going to attach it here.

[hsbt (Hiroshi SHIBATA)]

• Backport changed from 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED

• https://github.com/ruby/ruby/pull/4218
• https://github.com/ruby/ruby/pull/4219

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: DONE

Thank you for preparing backport PRs including bump a patchlevel.
I resolve conflict and merge into ruby_3_0 at 261a0e0e4a3202ca004eddc3cc2cefc9e8d0a90a.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.6: REQUIRED, 2.7: REQUIRED, 3.0: DONE to 2.6: REQUIRED, 2.7: DONE, 3.0: DONE

Backported into ruby_2_7 with resolving conflict :)