PTY: still problems (+patch)

Hi all,

I just built the lastest ruby distribution from CVS, and noticed that
PTY.spawn() does not throw a PTY::ChildExit exception after the process
death.

I’m running FreeBSD -current.

After some pty.c debug, I noticed that pty_finalize_syswait() was
called before pty_syswait(), thus the thread was killed before it
could call rb_waitpid().

Here is a short program I used to remark this problem::

<<----------------------------------------------------------------------
require './pty.so’
def myexec(cmd)
begin
PTY.spawn(cmd) do |r, w, pid|
r.each { |line| puts “-> #{line}” }
# sleep 1
end
rescue PTY::ChildExited => e
puts "STATUS: #{e.status}"
end
end
myexec(“uname”)
myexec(“ls /does_not_exist”)
myexec(“pleplrpaezlra”)
-------------------------------------------------------------------->>

It produces the following output:

-> FreeBSD
-> ls: /does_not_exist: No such file or directory

Note that if you uncomment the ``sleep 1’’ line, PTY.spawn will raise a
ChildExited object, which is the expected behaviour:

-> FreeBSD	
STATUS: 0
-> ls: /does_not_exist: No such file or directory
STATUS: 256
STATUS: 256

The following patch fixes this issue, by ‘joining’ the thread instead of killing it:

pinux@natalie ~/ruby/ext/pty> cvs diff -u pty.c
Index: pty.c

···

===================================================================
RCS file: /src/ruby/ext/pty/pty.c,v
retrieving revision 1.15
diff -u -r1.15 pty.c
— pty.c 10 Mar 2003 15:05:18 -0000 1.15
+++ pty.c 15 May 2003 20:24:38 -0000
@@ -299,7 +299,7 @@
pty_finalize_syswait(info)
struct pty_info *info;
{

  • rb_thread_kill(info->thread);
  • rb_funcall(info->thread, rb_intern(“join”), 0);
    rb_detach_process(info->child_pid);
    return Qnil;
    }

BTW, I saw that rb_thread_join() is private. Perhaps it should be
exported, in order to use its ‘limit’ parameter here.


Laurent

Hi,

After some pty.c debug, I noticed that pty_finalize_syswait() was
called before pty_syswait(), thus the thread was killed before it
could call rb_waitpid().

I noticed a bit different problem on Linux, EIO occurs.

The following patch fixes this issue, by ‘joining’ the thread instead of killing it:

And your patch also fixes it.

  • rb_thread_kill(info->thread);
  • rb_funcall(info->thread, rb_intern(“join”), 0);

Here, it might be better to use “value”.

BTW, I saw that rb_thread_join() is private. Perhaps it should be
exported, in order to use its ‘limit’ parameter here.

I agree to export it (and rb_thread_value maybe), but could you
explain why ‘limit’ is needed here?

···

At Fri, 16 May 2003 05:55:13 +0900, Laurent Sansonetti wrote:


Nobu Nakada

Hi Nobu,

I noticed a bit different problem on Linux, EIO occurs.

I can test the code on Linux, and help you to fix this problem.

The most difficult part here is to steal my girlfriend’s laptop :wink:

  • rb_thread_kill(info->thread);
  • rb_funcall(info->thread, rb_intern(“join”), 0);

Here, it might be better to use “value”.

You’re right, rb_thread_value() is meaningful here.

What do you think of the following patch?

pinux@natalie ~/ruby> cvs diff -u eval.c intern.h ext/pty/pty.c
Index: eval.c

···

nobu.nokada@softhome.net wrote:

RCS file: /src/ruby/eval.c,v
retrieving revision 1.430
diff -u -r1.430 eval.c
— eval.c 13 May 2003 05:53:08 -0000 1.430
+++ eval.c 16 May 2003 07:43:23 -0000
@@ -9178,7 +9178,7 @@
return rb_thread_start_0(rb_thread_yield, args,
rb_thread_alloc(klass));
}

-static VALUE
+VALUE
rb_thread_value(thread)
VALUE thread;
{
Index: intern.h

RCS file: /src/ruby/intern.h,v
retrieving revision 1.119
diff -u -r1.119 intern.h
— intern.h 13 May 2003 05:53:08 -0000 1.119
+++ intern.h 16 May 2003 07:43:24 -0000
@@ -212,6 +212,7 @@
VALUE rb_thread_local_aref _((VALUE, ID));
VALUE rb_thread_local_aset _((VALUE, ID, VALUE));
void rb_thread_atfork _((void));
+VALUE rb_thread_value _((VALUE));
/* file.c /
int eaccess _((const char
, int));
VALUE rb_file_s_expand_path _((int, VALUE *));
Index: ext/pty/pty.c

RCS file: /src/ruby/ext/pty/pty.c,v
retrieving revision 1.15
diff -u -r1.15 pty.c
— ext/pty/pty.c 10 Mar 2003 15:05:18 -0000 1.15
+++ ext/pty/pty.c 16 May 2003 07:43:26 -0000
@@ -299,7 +299,7 @@
pty_finalize_syswait(info)
struct pty_info *info;
{

  • rb_thread_kill(info->thread);
  • rb_thread_value(info->thread);
    rb_detach_process(info->child_pid);
    return Qnil;
    }

It works nicely here.

I agree to export it (and rb_thread_value maybe), but could you
explain why ‘limit’ is needed here?

After some reflection, there is no need to ‘limit’ the blocking. I was
a bit tired yesterday :wink:


Laurent

Hi,

···

At Fri, 16 May 2003 16:48:18 +0900, Laurent Sansonetti wrote:

What do you think of the following patch?

The modification itself seems good. But aren’t there extra
spaces at the beginning of unchanged lines?


Nobu Nakada

Hi Nobu,

The modification itself seems good. But aren’t there extra
spaces at the beginning of unchanged lines?

It is probably a problem with the ‘cut and paste’ operation in the
previous mail.

You can download the patch here:

http://lrz.is-a-geek.org/pty.patch

BTW, returning the status value in ChildExited is good. But it would be
more convenient to return the exit status value as well.

Actual code:

begin

rescue PTY::ChildExit => e
exit_status = e.status >> 8
end

I think it would be more user-friendly to add an accessor method in
pty.c (``exit_status’’ for example), which returns WEXITSTATUS(status).

Or a method in the Process module, like

module Process
def Process.exit_status(status)
status >> 8
end
end

begin

rescue PTY::ChildExit => e
exit_status = Process.exit_status(e.status)
end

The second option is better (I think), since it will be possible to call
Process.exit_status with Process.wait2()[1] (or Process.waitpid2(XXX)[1].

···

nobu.nokada@softhome.net wrote:


Laurent

Hi,

···

At Fri, 16 May 2003 17:35:53 +0900, Laurent Sansonetti wrote:

I think it would be more user-friendly to add an accessor method in
pty.c (``exit_status’’ for example), which returns WEXITSTATUS(status).

ChildExited#status returns Process::Status, and it has
#exitstatus, #pid and so on.


Nobu Nakada

Hi Nobu,

···

nobu.nokada@softhome.net wrote:

ChildExited#status returns Process::Status, and it has
#exitstatus, #pid and so on.

Again, I’m surprised that Ruby is very well designed :wink:

Do not hesitate to contact me if there is a problem with the patch.


Laurent