Bug #17394
closedTCPServer is not thread safe on win32
Description
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.
Files
Updated by puchuu (Andrew Aladjev) almost 4 years ago
PS This issue is not 100% reproducible, please try to run script several times, thank you.
Updated by puchuu (Andrew Aladjev) almost 4 years ago
I've added a screenshot from windows virtual machine on mingw64 env.
Updated by puchuu (Andrew Aladjev) over 3 years ago
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.
Updated by puchuu (Andrew Aladjev) over 3 years ago
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.
Updated by puchuu (Andrew Aladjev) over 3 years ago
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.
Updated by puchuu (Andrew Aladjev) over 3 years ago
Added fix https://github.com/ruby/ruby/pull/4212
Updated by jeremyevans0 (Jeremy Evans) over 3 years ago
- Status changed from Open to Closed
Fix merged at 0d76636117c99921ac7c43293ba7962d22e72fbd .
Updated by puchuu (Andrew Aladjev) over 3 years ago
- 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.
Updated by hsbt (Hiroshi SHIBATA) about 3 years ago
- Backport changed from 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED
Updated by nagachika (Tomoyuki Chikanaga) about 3 years ago
- 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.
Updated by usa (Usaku NAKAMURA) almost 3 years ago
- 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 :)