Bug #4683

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

Added by Eric Wong almost 3 years ago. Updated almost 3 years ago.

[ruby-core:36156]
Status:Closed
Priority:Normal
Assignee:Akira Tanaka
Category:core
Target version:1.9.3
ruby -v:ruby 1.9.3dev (2011-05-13 trunk 31504) [x86_64-linux] Backport:

Description

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 rbiowaitreadable() and rbiowaitwritable() retry
on interrupt (EAGAIN/ERESTART) instead of returning a short
copy or raising Errno::EINTR.

0001-io.c-copy_stream-execute-interrupts-and-retry.patch Magnifier - patch to fix the issue (4.02 KB) Eric Wong, 05/13/2011 06:56 AM

0002-test_io-add-test-for-copy_stream-failing-on-EINTR.patch Magnifier - test case, may not be portable... (1.75 KB) Eric Wong, 05/13/2011 06:56 AM

copy_stream_interrupt_handling.patch Magnifier (7.97 KB) Akira Tanaka, 05/29/2011 07:29 PM

copy_stream_interrupt_handling-2.patch Magnifier (9.42 KB) Akira Tanaka, 06/02/2011 08:20 PM


Related issues

Related to ruby-trunk - Bug #4680: [PATCH] io.c: fix busy wait with sendfile() Closed 05/13/2011

History

#1 Updated by Eric Wong almost 3 years ago

Eric Wong normalperson@yhbt.net 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 rbiowaitreadable() and rbiowaitwritable() 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

#2 Updated by Yui NARUSE almost 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to Akira Tanaka

#3 Updated by Akira Tanaka almost 3 years ago

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

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

Since the experimental function rbthreadcallwithgvl is not callable
with GVL, nogvlcopystreamcontinuep is not callable with GVL.
So nogvlcopystreamcontinuep is not callable from maygvl* such as
maygvl
copystreamwaitread.
I guess maygvl
* functions needs has_gvl parameter to keep GVL status.

#4 Updated by Akira Tanaka almost 3 years ago

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

#5 Updated by Akira Tanaka almost 3 years ago

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

#7 Updated by Eric Wong almost 3 years ago

Akira Tanaka akr@fsij.org wrote:

As far as I remember, io.c should not use vmcore.h and rbthread_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 rbthreadcallwithgvl() for
extensions:
http://redmine.ruby-lang.org/issues/4328

Perhaps execinterrupts() can be made into a public API method:
rb
threadexecinterruptswithgvl()

--
Eric Wong

#8 Updated by Akira Tanaka almost 3 years ago

Adding public functions needs a discussion with ko1.

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

#9 Updated by Eric Wong almost 3 years ago

Akira Tanaka akr@fsij.org wrote:

Issue #4683 has been updated by Akira Tanaka.

File copystreaminterrupt_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

#10 Updated by Akira Tanaka almost 3 years ago

  • Status changed from Assigned to Closed

Also available in: Atom PDF