Bug #4527: [PATCH] IO#close releases GVL if possible

• Status: Closed
• Assignee: kosaki (Motohiro KOSAKI)
• Target version: -
• ruby -v: ruby 1.9.3dev (2011-03-25 trunk 31181) [x86_64-linux]
• Backport: 2.0.0: DONTNEED, 2.1: DONTNEED

[normalperson (Eric Wong)]

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

[kosaki (Motohiro KOSAKI)]

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

Right. good catch.

[kosaki (Motohiro KOSAKI)]

• 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?

[normalperson (Eric Wong)]

Motohiro KOSAKI [e-mail] 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

[kosaki (Motohiro KOSAKI)]

• Status changed from Closed to Rejected

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

[normalperson (Eric Wong)]

Motohiro KOSAKI [e-mail] 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

[naruse (Yui NARUSE)]

• 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

[naruse (Yui NARUSE)]

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

[normalperson (Eric Wong)]

[e-mail] 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.

[naruse (Yui NARUSE)]

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

[e-mail] 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.