Rescue NoMethodError in TTYScreen#ioctl? #21
Conversation
**Background:** The `#ioctl?` method in `TTY::Screen` was only rescuing `SystemCallError`, causing unexpected exceptions to propagate when the method is called on streams that lack `#ioctl` support. This can happen with streams are set to IO objects like `StringIO`, as done by Minitest's `#capture_io` method. **Notable Changes:** - Add `NoMethodError` to the rescue clause in `#ioctl?` method - Add spec to verify behavior when output doesn't respond to `#ioctl`
piotrmurach
left a comment
piotrmurach
left a comment
There was a problem hiding this comment.
Thank you for reporting the issue and suggesting a fix. 🙏
I'm not sure about this change. When code replaces global constants at runtime, then all bets are off. Should this gem account for such scenarios? Even more so, calling the size method inside code that replaces such constants/streams as is the case with the capture_io method is redundant. You won't get actual measurements as there is no tty device attached. This will boil down to a default terminal size lookup. Given that the capture_io is a test method, it would be better to mock the size value.
Anyway, I left a couple of comments. Let me know what you think.
| ($stdin.ioctl(control, buf) >= 0) || | ||
| ($stderr.ioctl(control, buf) >= 0) | ||
| rescue SystemCallError | ||
| rescue SystemCallError, NoMethodError |
There was a problem hiding this comment.
The intention behind the ioctl? method isn't to check whether ioctl is supported. It assumes it is, and it checks whether any ioctl call on the standard streams can read information. So that's why only SystemCallError is being caught. Catching NoMethodError seems out of context here and confusing.
I believe a better approach and more direct would be to add the check in the size_from_ioctl method. That's what already happens at the module level.
def size_from_ioctl
return unless output.respond_to?(:ioctl)
...
end| end | ||
| end | ||
|
|
||
| it "doesn't detect size if the output doesn't respond to #iotcl" do |
There was a problem hiding this comment.
The test description isn't strictly correct. At the module level, there is a check to see whether output supports ioctl, failing that, it inlines the size_from_ioctl method body with nil implementation. The issue here is the runtime swapping of the output stream.
| it "doesn't detect size if the output doesn't respond to #iotcl" do | |
| it "doesn't detect size if the output changes to non-ioctl at runtime" do |
2 participants
Background:
The
#ioctl?method inTTY::Screenwas only rescuingSystemCallError, causing unexpected exceptions to propagate when the method is called on streams that lack#ioctlsupport.This can happen when streams are set to IO objects like
StringIO, as done by Minitest's#capture_iomethod.Notable Changes:
NoMethodErrorto the rescue clause in#ioctl?method#ioctl