Project

General

Profile

Backport #5539

Readline.readline() blocks all threads

Added by cjameshuff (Christopher Huff) about 8 years ago. Updated almost 8 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
[ruby-core:40641]

Description

The standard library Readline.readline() function blocks the thread it's called from without unlocking the VM, thus blocking all threads. This code demonstrates the problem:

require 'pp'
t = Thread.new {10.times {|x| puts x; sleep 1}}
while(buf = Readline.readline)
p buf
if(buf == 'q')
break
end
end

Thread t will be blocked by the readline call, numbers being printed occasionally when lines are entered and readline() returns, rather than once per second. Reproduced on ruby 1.9.2p180 (2011-02-18 revision 30909) [x86_64-darwin10.6.0] and ruby 1.9.3p0 (2011-10-30 revision 33570) [x86_64-darwin10.8.0].

The RubyInline gem allows a workaround:

require 'inline'
class BetterReadline
inline :C do |builder|
builder.include ''
builder.include ''
src = <<END
VALUE readline_intern(void * data) {
char ** rstr = (char **)data;
*rstr = readline(NULL);
return Qnil;
}
END
builder.prefix(src)
src = <<END
VALUE rb_readline(void) {
char * str = NULL;
rb_thread_blocking_region(readline_intern, &str, NULL, NULL);
return rb_str_new2(str);
}
END
builder.c_singleton(src, method_name: 'readline')
end
end # class BetterReadline

This then illustrates the desired behavior, with the thread running in the background while readline() waits for input:

require 'pp'
t = Thread.new {10.times {|x| puts x; sleep 1}}
while(buf = BetterReadline.readline)
p buf
if(buf == 'q')
break
end
end


Files

readline.patch (1009 Bytes) readline.patch nagachika (Tomoyuki Chikanaga), 11/04/2011 11:46 PM
readline_comment.patch (1.18 KB) readline_comment.patch nagachika (Tomoyuki Chikanaga), 11/08/2011 12:24 AM

Associated revisions

Revision ec4f9d1c
Added by kouji (Kouji Takao) almost 8 years ago

  • ext/readline/readline.c (Init_readline): libedit check rl_getc_function only when rl_initialize() is called, and using_history() call rl_initialize(). This assignment should be placed before using_history(). [ruby-core:40641] [Bug #5539]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@34108 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 34108
Added by kouji (Kouji Takao) almost 8 years ago

  • ext/readline/readline.c (Init_readline): libedit check rl_getc_function only when rl_initialize() is called, and using_history() call rl_initialize(). This assignment should be placed before using_history(). [ruby-core:40641] [Bug #5539]

Revision 34108
Added by kouji (Kouji Takao) almost 8 years ago

  • ext/readline/readline.c (Init_readline): libedit check rl_getc_function only when rl_initialize() is called, and using_history() call rl_initialize(). This assignment should be placed before using_history(). [ruby-core:40641] [Bug #5539]

Revision 34108
Added by kouji (Kouji Takao) almost 8 years ago

  • ext/readline/readline.c (Init_readline): libedit check rl_getc_function only when rl_initialize() is called, and using_history() call rl_initialize(). This assignment should be placed before using_history(). [ruby-core:40641] [Bug #5539]

Revision 34108
Added by kouji (Kouji Takao) almost 8 years ago

  • ext/readline/readline.c (Init_readline): libedit check rl_getc_function only when rl_initialize() is called, and using_history() call rl_initialize(). This assignment should be placed before using_history(). [ruby-core:40641] [Bug #5539]

Revision 34108
Added by kouji (Kouji Takao) almost 8 years ago

  • ext/readline/readline.c (Init_readline): libedit check rl_getc_function only when rl_initialize() is called, and using_history() call rl_initialize(). This assignment should be placed before using_history(). [ruby-core:40641] [Bug #5539]

Revision 34108
Added by kouji (Kouji Takao) almost 8 years ago

  • ext/readline/readline.c (Init_readline): libedit check rl_getc_function only when rl_initialize() is called, and using_history() call rl_initialize(). This assignment should be placed before using_history(). [ruby-core:40641] [Bug #5539]

Revision 0239882c
Added by kosaki (Motohiro KOSAKI) almost 8 years ago

merge revision(s) 34108:

    * ext/readline/readline.c (Init_readline): libedit check
      rl_getc_function only when rl_initialize() is called, and
      using_history() call rl_initialize(). This assignment should be
      placed before using_history(). [ruby-core:40641] [Bug #5539]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_1_9_3@34196 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 34196
Added by kosaki (Motohiro KOSAKI) almost 8 years ago

merge revision(s) 34108:

* ext/readline/readline.c (Init_readline): libedit check
  rl_getc_function only when rl_initialize() is called, and
  using_history() call rl_initialize(). This assignment should be
  placed before using_history(). [ruby-core:40641] [Bug #5539]

History

Updated by cjameshuff (Christopher Huff) about 8 years ago

The issue pops up when Ruby uses the Editline library instead of GNU Readline. I suspect it is actually some conditionally compiled code that depends on the library being used, not the readline() call itself, since both the above workaround and GNU Readline work as expected.

Updated by nagachika (Tomoyuki Chikanaga) about 8 years ago

Hi,

In my environments, with GNU readline, your sample code work as expected.
I've found that a variable rl_getc_function is pointed readline_getc() and readline_getc() call rb_funcall() to invoke IO#getbyte method. Obviously the IO#getbyte release GVL during blocking.
I guess editline don't provide global variable rl_getc_function to customize getc function.
I think your workaround is potentially conflicted with rl_getc_function because it end up to call ruby method without GVL.

Thanks,

Updated by nagachika (Tomoyuki Chikanaga) about 8 years ago

Hi,

I'm sorry my previous estimate was wrong. nakada san told me that libeditline also has `rl_getc_function'. There should be another cause to prevent readline_getc() works as expected.
I'll examine it later.

Thanks,

Updated by nagachika (Tomoyuki Chikanaga) about 8 years ago

I can reproduce the issue wiht readline configured with --enable-libedit option.
I wrote a patch.

Takao san, how about this patch?

Description of the patch:
libedit check rl_getc_function only when rl_initialize() is called. And using_history() may call rl_initialize(). So rl_getc_function should be assigned before calling using_history().

Thanks,

Updated by nagachika (Tomoyuki Chikanaga) about 8 years ago

Obviously, the patch should contains comments about the constraint of the code sequence. Updated the patch.

Updated by nagachika (Tomoyuki Chikanaga) about 8 years ago

Takao san,
Is it hard to make time to take a look at this issue?

Thanks,

#7

Updated by kouji (Kouji Takao) almost 8 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r34108.
Christopher, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • ext/readline/readline.c (Init_readline): libedit check rl_getc_function only when rl_initialize() is called, and using_history() call rl_initialize(). This assignment should be placed before using_history(). [ruby-core:40641] [Bug #5539]
#8

Updated by kouji (Kouji Takao) almost 8 years ago

  • Tracker changed from Bug to Backport
  • Project changed from Ruby master to Backport193
  • Category deleted (ext)
  • Status changed from Closed to Open
  • Assignee deleted (kouji (Kouji Takao))

Please backport r34108 to ruby_1_9_3

#9

Updated by kosaki (Motohiro KOSAKI) almost 8 years ago

  • Status changed from Open to Closed

This issue was solved with changeset r34196.
Christopher, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


merge revision(s) 34108:

* ext/readline/readline.c (Init_readline): libedit check
  rl_getc_function only when rl_initialize() is called, and
  using_history() call rl_initialize(). This assignment should be
  placed before using_history(). [ruby-core:40641] [Bug #5539]

Also available in: Atom PDF