-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle INT signal correctly #646
Conversation
276e1ca
to
70cba12
Compare
lib/reline/ansi.rb
Outdated
@@ -278,7 +284,7 @@ def self.cursor_pos | |||
buf = @@output.pread(@@output.pos, 0) | |||
row = buf.count("\n") | |||
column = buf.rindex("\n") ? (buf.size - buf.rindex("\n")) - 1 : 0 | |||
rescue Errno::ESPIPE | |||
rescue Errno::ESPIPE, IOError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my learning: Why did you need IOError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is because pread
raises IOError in some situation #537.
Removed this change because it is out of scope of this pull request.
Reline::HISTORY << whole_buffer | ||
end | ||
whole_buffer = line_editor.whole_buffer.dup | ||
whole_buffer.taint if RUBY_VERSION < '2.7' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the taint
call necessary for Ruby 2.6? If not, I think it's fine to remove it now as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kernel#gets
and other string read from IO are tainted in Ruby 2.6 https://docs.ruby-lang.org/en/2.6.0/Object.html#method-i-taint
Reline should return tainted string to align with Kernel.gets
.
I'm not sure of the impact of removing taint. Perhaps nobody is using tainted?
, but I'd like to keep this until dropping 2.6 support.
# Code relying on taint security model
Foo::Settings.map do |s|
if s.tainted?
s=~/\A\d+\z/ ? s.to_i : s
else
eval s
end
end
lib/reline/line_editor.rb
Outdated
@@ -1089,6 +1089,7 @@ def input_key(key) | |||
end | |||
end | |||
if key.char.nil? | |||
process_insert(force: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is process_insert
needed in key.char.nil? == true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed but not related to this pull request. I'll open another one with test code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is failing, but I think it isn't because of the code change. LGTM!
because clear(C-l) is not a signal (ruby/reline#646) ruby/reline@3debb0ae2f
because clear(C-l) is not a signal (ruby/reline#646) ruby/reline@3debb0ae2f
Handle SIGINT and SIGWINCH correctly, (just update ivar)
inner_getc
will periodically check if these signal is fired.Changed signal check timeout from 0.1 to 0.01.
C-c
is handled here, so 0.1s is too slow. (C-c will delay 0.1s)Remove handle_cleared because clear(C-l) is not a signal
This will fix #461
This will fix C-c problem when
binding.irb
is executed in non-main thread.ruby -e "Thread.new{binding.irb}.join"
Another possible way to correctly handle SIGINT is to use
input.raw{getc == "\C-c"}
instead of usingSignal.trap
andinput.raw(intr:true){}
.But we have to handle other signals too. (example:
\C-z
). We also have to think for windows.Note: This pull request DOES NOT fix multiple readline in multithread problem
2.times{Thread.new{Reline.readline'>'}}