Bug #4683: [PATCH] io.c: copy_stream execute interrupts and retry

• Status: Closed
• Assignee: akr (Akira Tanaka)
• Target version: 1.9.3
• ruby -v: ruby 1.9.3dev (2011-05-13 trunk 31504) [x86_64-linux]

[normalperson (Eric Wong)]

It's debatable whether this is a bug or not, but I think the current interrupt
handling behavior with IO.copy_stream is fragile and unpredictable, and
inconsistent with IO#read and IO#write.

This is to be consistent with IO#read and IO#write behavior
where rb_io_wait_readable() and rb_io_wait_writable() retry
on interrupt (EAGAIN/ERESTART) instead of returning a short
copy or raising Errno::EINTR.

[normalperson (Eric Wong)]

Eric Wong [e-mail] wrote:

Bug #4683: [PATCH] io.c: copy_stream execute interrupts and retry
http://redmine.ruby-lang.org/issues/4683

It's debatable whether this is a bug or not, but I think the current interrupt
handling behavior with IO.copy_stream is fragile and unpredictable, and
inconsistent with IO#read and IO#write.

This is to be consistent with IO#read and IO#write behavior
where rb_io_wait_readable() and rb_io_wait_writable() retry
on interrupt (EAGAIN/ERESTART) instead of returning a short
copy or raising Errno::EINTR.

Can I get a response on this soon? I really want this issue resolved
before 1.9.3 because it's inconsistent with existing Ruby read/write
behavior and working around it painful.

A backport to 1.9.2 would be appreciated, too.

Thanks!

--
Eric Wong

[naruse (Yui NARUSE)]

• Status changed from Open to Assigned
• Assignee set to akr (Akira Tanaka)

[akr (Akira Tanaka)]

I think this is subtle but interesting issue. Thank you.

I'm not sure rb_thread_interrupted() is callable without GVL.
kosaki pointed mutual exclusion issue with interrupt_flag.
http://redmine.ruby-lang.org/issues/4770

Since the experimental function rb_thread_call_with_gvl is not callable
with GVL, nogvl_copy_stream_continue_p is not callable with GVL.
So nogvl_copy_stream_continue_p is not callable from maygvl_* such as
maygvl_copy_stream_wait_read.
I guess maygvl_* functions needs has_gvl parameter to keep GVL status.

[akr (Akira Tanaka)]

Hm. rb_thread_interrupted() is safe to call from blocking region.

[akr (Akira Tanaka)]

As far as I remember, io.c should not use vm_core.h and rb_thread_t.
ko1 dislikes it.

[akr (Akira Tanaka)]

• File copy_stream_interrupt_handling.patch copy_stream_interrupt_handling.patch added

[normalperson (Eric Wong)]

Akira Tanaka [e-mail] wrote:

As far as I remember, io.c should not use vm_core.h and rb_thread_t.
ko1 dislikes it.

I think we can work around it by making more API methods public.

I have an old issue open to export rb_thread_call_with_gvl() for
extensions:
http://redmine.ruby-lang.org/issues/4328

Perhaps exec_interrupts() can be made into a public API method:
rb_thread_exec_interrupts_with_gvl()

--
Eric Wong

[akr (Akira Tanaka)]

• File copy_stream_interrupt_handling-2.patch copy_stream_interrupt_handling-2.patch added

Adding public functions needs a discussion with ko1.

copy_stream_interrupt_handling-2.patch uses internal.h to share
declarations between io.c and thread.c.

[normalperson (Eric Wong)]

Akira Tanaka [e-mail] wrote:

Issue #4683 has been updated by Akira Tanaka.

File copy_stream_interrupt_handling-2.patch added

Patch looks good, thanks! Can you please commit?

Adding public functions needs a discussion with ko1.

I haven't seen any response for #4328 in months, though :(

--
Eric Wong

[akr (Akira Tanaka)]

• Status changed from Assigned to Closed