Project

General

Profile

Actions

Bug #17394

closed

TCPServer is not thread safe on win32

Added by puchuu (Andrew Aladjev) almost 4 years ago. Updated almost 3 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:101458]

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

Screenshot_20201215_020316.png (11.4 KB) Screenshot_20201215_020316.png puchuu (Andrew Aladjev), 12/14/2020 11:03 PM
4220.patch (7.68 KB) 4220.patch Backport mutexes for socket and connection lists on win32 puchuu (Andrew Aladjev), 02/23/2021 10:49 PM

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.

Actions #3

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.

Actions #4

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.

Actions #5

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 jeremyevans0 (Jeremy Evans) over 3 years ago

  • Status changed from Open to Closed

Updated by puchuu (Andrew Aladjev) over 3 years ago

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 :)

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0