ruby-changes:40070
From: kosaki <ko1@a...>
Date: Sun, 18 Oct 2015 06:09:32 +0900 (JST)
Subject: [ruby-changes:40070] kosaki:r52151 (trunk): * ruby.c (open_load_file): reset O_NONBLOCK after open.
kosaki 2015-10-18 06:09:10 +0900 (Sun, 18 Oct 2015) New Revision: 52151 http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=52151 Log: * ruby.c (open_load_file): reset O_NONBLOCK after open. Even if S_ISREG() is true, the file may be file on FUSE filesystem or something. We can't assume O_NONBLOCK is safe. Moreover, we should wait if the path is point to FIFO. That's FIFO semantics. GVL should be transparent from ruby script. Thus, just reopen without O_NONBLOCK for filling the requirements. [Bug #11060][Bug #11559] * ruby.c (loadopen_func): new for the above. * file.c (ruby_is_fd_loadable): new. for checks loadable file type of not. * file.c (rb_file_load_ok): use ruby_is_fd_loadble() * internal.h: add ruby_is_fd_loadble() * common.mk: now, ruby.o depend on thread.h. * test/ruby/test_require.rb (TestRequire#test_loading_fifo_threading_success): new test. This test successful case that loading from FIFO. * test/ruby/test_require.rb (TestRequire#test_loading_fifo_threading_raise): rename from test_loading_fifo_threading. You souldn't rescue an exception if you test raise or not. Moreover, this case should be caught IOError because load(FIFO) should be blocked until given any input. Modified files: trunk/ChangeLog trunk/common.mk trunk/file.c trunk/internal.h trunk/ruby.c trunk/test/ruby/test_require.rb Index: ChangeLog =================================================================== --- ChangeLog (revision 52150) +++ ChangeLog (revision 52151) @@ -1,3 +1,33 @@ https://github.com/ruby/ruby/blob/trunk/ChangeLog#L1 +Sun Oct 18 05:11:22 2015 KOSAKI Motohiro <kosaki.motohiro@g...> + + * ruby.c (open_load_file): reset O_NONBLOCK after open. + Even if S_ISREG() is true, the file may be file on FUSE filesystem + or something. We can't assume O_NONBLOCK is safe. + Moreover, we should wait if the path is point to FIFO. That's + FIFO semantics. GVL should be transparent from ruby script. + Thus, just reopen without O_NONBLOCK for filling the requirements. + [Bug #11060][Bug #11559] + + * ruby.c (loadopen_func): new for the above. + + * file.c (ruby_is_fd_loadable): new. for checks loadable file type + of not. + * file.c (rb_file_load_ok): use ruby_is_fd_loadble() + * internal.h: add ruby_is_fd_loadble() + + * common.mk: now, ruby.o depend on thread.h. + + * test/ruby/test_require.rb + (TestRequire#test_loading_fifo_threading_success): new test. + This test successful case that loading from FIFO. + + * test/ruby/test_require.rb + (TestRequire#test_loading_fifo_threading_raise): rename from + test_loading_fifo_threading. You souldn't rescue an exception + if you test raise or not. + Moreover, this case should be caught IOError because load(FIFO) + should be blocked until given any input. + Sat Oct 17 13:55:32 2015 Nobuyoshi Nakada <nobu@r...> * file.c (rb_file_expand_path_internal): concatenate converted Index: common.mk =================================================================== --- common.mk (revision 52150) +++ common.mk (revision 52151) @@ -2062,6 +2062,7 @@ ruby.$(OBJEXT): {$(VPATH)}ruby.c https://github.com/ruby/ruby/blob/trunk/common.mk#L2062 ruby.$(OBJEXT): {$(VPATH)}ruby_atomic.h ruby.$(OBJEXT): {$(VPATH)}st.h ruby.$(OBJEXT): {$(VPATH)}subst.h +ruby.$(OBJEXT): {$(VPATH)}thread.h ruby.$(OBJEXT): {$(VPATH)}thread_$(THREAD_MODEL).h ruby.$(OBJEXT): {$(VPATH)}thread_native.h ruby.$(OBJEXT): {$(VPATH)}util.h Index: internal.h =================================================================== --- internal.h (revision 52150) +++ internal.h (revision 52151) @@ -762,6 +762,7 @@ VALUE rb_file_expand_path_internal(VALUE https://github.com/ruby/ruby/blob/trunk/internal.h#L762 VALUE rb_get_path_check_to_string(VALUE, int); VALUE rb_get_path_check_convert(VALUE, VALUE, int); void Init_File(void); +int ruby_is_fd_loadable(int fd); #ifdef RUBY_FUNCTION_NAME_STRING # if defined __GNUC__ && __GNUC__ >= 4 Index: ruby.c =================================================================== --- ruby.c (revision 52150) +++ ruby.c (revision 52151) @@ -16,6 +16,7 @@ https://github.com/ruby/ruby/blob/trunk/ruby.c#L16 #include <sys/cygwin.h> #endif #include "internal.h" +#include "ruby/thread.h" #include "eval_intern.h" #include "dln.h" #include <stdio.h> @@ -1725,21 +1726,36 @@ load_file_internal(VALUE argp_v) https://github.com/ruby/ruby/blob/trunk/ruby.c#L1726 return (VALUE)tree; } +static void * +loadopen_func(void *arg) +{ + int fd; + fd = rb_cloexec_open((const char *)arg, O_RDONLY, 0); + if (fd >= 0) + rb_update_max_fd(fd); + + return (void *)(VALUE)fd; +} + static VALUE open_load_file(VALUE fname_v, int *xflag) { const char *fname = StringValueCStr(fname_v); VALUE f; + int e; if (RSTRING_LEN(fname_v) == 1 && fname[0] == '-') { f = rb_stdin; } else { - int fd, mode = O_RDONLY; -#if defined O_NONBLOCK && !(O_NONBLOCK & O_ACCMODE) + int fd; + /* open(2) may block if fname is point to FIFO and it's empty. Let's + use O_NONBLOCK. */ + int mode = O_RDONLY; +#if defined O_NONBLOCK && HAVE_FCNTL && !(O_NONBLOCK & O_ACCMODE) /* TODO: fix conflicting O_NONBLOCK in ruby/win32.h */ mode |= O_NONBLOCK; -#elif defined O_NDELAY && !(O_NDELAY & O_ACCMODE) +#elif defined O_NDELAY && HAVE_FCNTL && !(O_NDELAY & O_ACCMODE) mod |= O_NDELAY; #endif #if defined DOSISH || defined __CYGWIN__ @@ -1751,21 +1767,44 @@ open_load_file(VALUE fname_v, int *xflag https://github.com/ruby/ruby/blob/trunk/ruby.c#L1767 } } #endif + if ((fd = rb_cloexec_open(fname, mode, 0)) < 0) { rb_load_fail(fname_v, strerror(errno)); } - rb_update_max_fd(fd); -#if !defined DOSISH && !defined __CYGWIN__ + rb_update_max_fd(fd); + +#ifdef HAVE_FCNTL + /* disabling O_NONBLOCK */ + if (fcntl(fd, F_SETFL, 0) < 0) { + e = errno; + close(fd); + rb_load_fail(fname_v, strerror(e)); + } +#endif + +#ifdef S_ISFIFO { struct stat st; - int e; - if ((fstat(fd, &st) != 0) && (e = errno, 1) || - (S_ISDIR(st.st_mode) && (e = EISDIR, 1))) { - (void)close(fd); + if (fstat(fd, &st) != 0) { + e = errno; + close(fd); rb_load_fail(fname_v, strerror(e)); } + if (S_ISFIFO(st.st_mode)) { + /* We need to wait if FIFO is empty. So, let's reopen it. */ + close(fd); + fd = (int)(VALUE)rb_thread_call_without_gvl(loadopen_func, + (void *)fname, RUBY_UBF_IO, 0); + if (fd < 0) + rb_load_fail(fname_v, strerror(errno)); + } } #endif + if (!ruby_is_fd_loadable(fd)) { + close(fd); + rb_load_fail(fname_v, strerror(errno)); + } + f = rb_io_fdopen(fd, mode, fname); } return f; Index: test/ruby/test_require.rb =================================================================== --- test/ruby/test_require.rb (revision 52150) +++ test/ruby/test_require.rb (revision 52151) @@ -694,7 +694,7 @@ class TestRequire < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_require.rb#L694 } end - def test_loading_fifo_threading + def test_loading_fifo_threading_raise Tempfile.create(%w'fifo .rb') {|f| f.close File.unlink(f.path) @@ -702,16 +702,39 @@ class TestRequire < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_require.rb#L702 assert_separately(["-", f.path], <<-END, timeout: 3) th = Thread.current Thread.start {begin sleep(0.001) end until th.stop?; th.raise(IOError)} - assert_nothing_raised do - begin - load(ARGV[0]) - rescue IOError - end + assert_raise(IOError) do + load(ARGV[0]) end END } end unless /mswin|mingw/ =~ RUBY_PLATFORM + def test_loading_fifo_threading_success + Tempfile.create(%w'fifo .rb') {|f| + f.close + File.unlink(f.path) + File.mkfifo(f.path) + + assert_separately(["-", f.path], <<-INPUT, timeout: 3) + path = ARGV[0] + th = Thread.current + Thread.start { + begin + sleep(0.001) + end until th.stop? + open(path, File::WRONLY | File::NONBLOCK) {|fifo_w| + fifo_w.puts "class C1; FOO='foo'; end" + } + } + + load(path) + assert_equal(C1::FOO, "foo") + INPUT + } + + end unless /mswin|mingw/ =~ RUBY_PLATFORM + + def test_throw_while_loading Tempfile.create(%w'bug-11404 .rb') do |f| f.puts 'sleep' Index: file.c =================================================================== --- file.c (revision 52150) +++ file.c (revision 52151) @@ -5670,11 +5670,35 @@ rb_path_check(const char *path) https://github.com/ruby/ruby/blob/trunk/file.c#L5670 return 1; } +int +ruby_is_fd_loadable(int fd) +{ +#ifdef _WIN32 + return 1; +#else + struct stat st; + + if (fstat(fd, &st) < 0) + return 0; + + if (S_ISREG(st.st_mode)) + return 1; + if (S_ISFIFO(st.st_mode)) + return 1; + + return 0; +#endif +} + #ifndef _WIN32 int rb_file_load_ok(const char *path) { int ret = 1; + /* + open(2) may block if path is FIFO and it's empty. Let's use O_NONBLOCK. + FIXME: Why O_NDELAY is checked? + */ int mode = (O_RDONLY | #if defined O_NONBLOCK O_NONBLOCK | @@ -5685,14 +5709,7 @@ rb_file_load_ok(const char *path) https://github.com/ruby/ruby/blob/trunk/file.c#L5709 int fd = rb_cloexec_open(path, mode, 0); if (fd == -1) return 0; rb_update_max_fd(fd); -#if !defined DOSISH - { - struct stat st; - if (fstat(fd, &st) || S_ISDIR(st.st_mode)) { - ret = 0; - } - } -#endif + ret = ruby_is_fd_loadable(fd); (void)close(fd); return ret; } -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/