Project

General

Profile

Bug #4527

[PATCH] IO#close releases GVL if possible

Added by normalperson (Eric Wong) over 9 years ago. Updated about 6 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
ruby 1.9.3dev (2011-03-25 trunk 31181) [x86_64-linux]
[ruby-core:35555]

Description

=begin
close() may block for certain file types (NFS, SO_LINGER
sockets, inotify), so let other threads run.
=end


Files

0001-IO-close-releases-GVL-if-possible.patch (1.6 KB) 0001-IO-close-releases-GVL-if-possible.patch [PATCH] IO#close releases GVL if possible normalperson (Eric Wong), 03/26/2011 03:35 AM

Related issues

Related to Ruby master - Bug #4558: TestSocket#test_closed_read fails after r31230Closedkosaki (Motohiro KOSAKI)04/07/2011Actions
Related to Ruby master - Feature #4570: [PATCH v2] io.c (rb_io_close): release GVL if possibleClosedkosaki (Motohiro KOSAKI)04/12/2011Actions

Updated by kosaki (Motohiro KOSAKI) over 9 years ago

  • Status changed from Open to Assigned
  • Assignee set to kosaki (Motohiro KOSAKI)

Right. good catch.

Updated by kosaki (Motohiro KOSAKI) over 9 years ago

  • Status changed from Assigned to Closed

Eric, I commited slightly modified version because your patch couldn't apply for latest trunk. Could you please confirm it?

Updated by normalperson (Eric Wong) over 9 years ago

Motohiro KOSAKI kosaki.motohiro@gmail.com wrote:

Eric, I commited slightly modified version because your patch couldn't
apply for latest trunk. Could you please confirm it?

Thanks, looks like you missed the following hunk in rb_io_reopen():

diff --git a/io.c b/io.c
index 7ce7148..0c1593b 100644
--- a/io.c
+++ b/io.c
@@ -5930,7 +5930,7 @@ rb_io_reopen(int argc, VALUE *argv, VALUE file)
 #endif
     }
     else {
-        if (close(fptr->fd) < 0)
+        if (maygvl_close(fptr->fd, 0) < 0)
             rb_sys_fail_path(fptr->pathv);
         fptr->fd = -1;
         fptr->fd = rb_sysopen(fptr->pathv, oflags, 0666);

Eric Wong

Updated by kosaki (Motohiro KOSAKI) over 9 years ago

  • Status changed from Closed to Rejected

The patch reverted because it made a regression (see [Bug #4558])

Updated by normalperson (Eric Wong) over 9 years ago

Motohiro KOSAKI kosaki.motohiro@gmail.com wrote:

Eric, I commited slightly modified version because your patch couldn't
apply for latest trunk. Could you please confirm it?

Thanks, looks like you missed the following hunk in rb_io_reopen():

diff --git a/io.c b/io.c
index 7ce7148..0c1593b 100644
--- a/io.c
+++ b/io.c
@@ -5930,7 +5930,7 @@ rb_io_reopen(int argc, VALUE *argv, VALUE file)
 #endif
     }
     else {
-        if (close(fptr->fd) < 0)
+        if (maygvl_close(fptr->fd, 0) < 0)
             rb_sys_fail_path(fptr->pathv);
         fptr->fd = -1;
         fptr->fd = rb_sysopen(fptr->pathv, oflags, 0666);

Eric Wong

Updated by naruse (Yui NARUSE) about 6 years ago

  • Description updated (diff)
  • Status changed from Rejected to Assigned
  • Target version deleted (2.0.0)
  • Backport set to 2.0.0: REQUIRED, 2.1: REQUIRED

Updated by naruse (Yui NARUSE) about 6 years ago

A patch proposed Eric in [ruby-core:35610] is not merged yet.

Updated by normalperson (Eric Wong) about 6 years ago

naruse@airemix.jp wrote:

A patch proposed Eric in [ruby-core:35610] is not merged yet.

Won't apply after r43373 (which is way more important).

I don't think the current close(tmpfd) needs to release GVL in the
new code path it is dropping a refcount after rb_cloexec_dup2;
so nothing expensive going on in the kernel.

close performance on inotify descriptors (my main reason for this) is
much improved since 2011, too. AFAIK the heavy lifting was moved to
asynchronous kernel workqueues.

Updated by naruse (Yui NARUSE) about 6 years ago

  • Status changed from Assigned to Closed
  • Backport changed from 2.0.0: REQUIRED, 2.1: REQUIRED to 2.0.0: DONTNEED, 2.1: DONTNEED

Eric Wong wrote:

naruse@airemix.jp wrote:

A patch proposed Eric in [ruby-core:35610] is not merged yet.

Won't apply after r43373 (which is way more important).

I don't think the current close(tmpfd) needs to release GVL in the
new code path it is dropping a refcount after rb_cloexec_dup2;
so nothing expensive going on in the kernel.

close performance on inotify descriptors (my main reason for this) is
much improved since 2011, too. AFAIK the heavy lifting was moved to
asynchronous kernel workqueues.

Sure, thanks for explanation.

Also available in: Atom PDF