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

Fix an infinite loop when attempting to read from a directory #4817

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@philr
Contributor

philr commented Oct 15, 2017

When attempting to open a directory with File.open and then read from it, JRuby 9.1.13.0 (on platforms other than Windows) will enter an infinite loop and hang:

> Timeout::timeout(10) { File.open('.') {|f| f.read } }
Timeout::Error: execution expired
        from org/jruby/RubyIO.java:3009:in `read'
        from org/jruby/RubyIO.java:2987:in `read'
        from (irb):1:in `block in evaluate'
        from org/jruby/RubyIO.java:1156:in `open'
        from (irb):1:in `block in evaluate'
        from org/jruby/ext/timeout/Timeout.java:117:in `timeout'
        from (irb):1:in `<eval>'

When attempting to do the same with MRI (tested with versions 1.9.3 through to 2.4.2) an Errno::EISDIR exception is raised:

> File.open('.') {|f| f.read }
Errno::EISDIR: Is a directory @ io_fread - .
        from (irb):1:in `read'
        from (irb):1:in `block in irb_binding'
        from (irb):1:in `open'
        from (irb):1

The hang occurs because OpenFile.fillbuf() and OpenFile.ioBufread() loop to retry on errors. This is desirable for temporary errors like EAGAIN, but not for permanent errors like EISDIR.

This pull request modifies OpenFile.waitReadable() to abort retries when a permanent error is encountered. It also changes Helpers.errnoFromException() to map the "Is a directory" IOException message to EISDIR.

This change only applies to non-Windows platforms. On Windows, File.open raises an exception instead.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 23, 2017

Member

have tried running this against jruby-9.1 but this time there seems to be more failures from test:jruby
... than before (last jruby-9.1 build here) : https://travis-ci.org/kares/jruby/jobs/291393645

Member

kares commented Oct 23, 2017

have tried running this against jruby-9.1 but this time there seems to be more failures from test:jruby
... than before (last jruby-9.1 build here) : https://travis-ci.org/kares/jruby/jobs/291393645

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr Oct 28, 2017

Contributor

@kares I'm seeing the same test failures from test:jruby in the build of the merge of this change to your jruby-9.1 branch (https://travis-ci.org/kares/jruby/jobs/291393645) as in the build of the parent commit (https://travis-ci.org/jruby/jruby/jobs/289840028). The following tests fail in both builds:

  • test_ipv6_loopback?
  • test_directory_query
  • test_executable?_query
  • test_executable_real?_query
  • test_file_exist_query
  • test_file_query
  • test_readable_query

I've just merged this onto jruby-9.1 myself (philr/jruby@5d25f7b). Running test:jruby in my dev environment, I only see the test_ipv6_loopback? failure. This is the same outcome as the latest jruby-9.1 build (https://travis-ci.org/jruby/jruby/jobs/293860475).

Contributor

philr commented Oct 28, 2017

@kares I'm seeing the same test failures from test:jruby in the build of the merge of this change to your jruby-9.1 branch (https://travis-ci.org/kares/jruby/jobs/291393645) as in the build of the parent commit (https://travis-ci.org/jruby/jruby/jobs/289840028). The following tests fail in both builds:

  • test_ipv6_loopback?
  • test_directory_query
  • test_executable?_query
  • test_executable_real?_query
  • test_file_exist_query
  • test_file_query
  • test_readable_query

I've just merged this onto jruby-9.1 myself (philr/jruby@5d25f7b). Running test:jruby in my dev environment, I only see the test_ipv6_loopback? failure. This is the same outcome as the latest jruby-9.1 build (https://travis-ci.org/jruby/jruby/jobs/293860475).

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 29, 2017

Member

oh really? have assumed they're builds from the same base, although I did not look carefully ...
hard to work with CI failures 😭 - but since jruby-9.1 only has the one let's have it for 9.1.14.0

Member

kares commented Oct 29, 2017

oh really? have assumed they're builds from the same base, although I did not look carefully ...
hard to work with CI failures 😭 - but since jruby-9.1 only has the one let's have it for 9.1.14.0

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Oct 29, 2017

Member

done at 963e2a1 - thanks for the fix

Member

kares commented Oct 29, 2017

done at 963e2a1 - thanks for the fix

@kares kares closed this Oct 29, 2017

@kares kares added this to the JRuby 9.1.14.0 milestone Oct 29, 2017

@philr philr deleted the philr:fix_infinite_loop_reading_directory branch Oct 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment