Process spawn should not raise NoMethodError when true argument is given to :pgroup #1895

Merged
merged 2 commits into from Sep 22, 2012

Projects

None yet

2 participants

@LTe

No description provided.

@dbussink dbussink and 1 other commented on an outdated diff Sep 11, 2012
spec/ruby/shared/process/spawn.rb
@@ -268,6 +268,10 @@
it "raises an ArgumentError if given a negative :pgroup option" do
lambda { @object.spawn("echo", :pgroup => -1) }.should raise_error(ArgumentError)
end
+
+ it "does not raises an NoMethodError if given a true :pgroup option" do
+ lambda { @object.spawn("echo -n", :pgroup => true) }.should_not raise_error(NoMethodError)
@dbussink
dbussink Sep 11, 2012

We should try to spec behavior here as how it should work, not that it shouldn't raise an exception. So that means we should specify here that it creates a new pgroup.

In general, any spec that says "should not raise an exception" is almost always a wrong spec.

@LTe
LTe Sep 11, 2012

I update pull request. And spec about "should not raise and exception" can be good: LTe/rubinius@a16eeac#L0R272 :)

@LTe

Process.spawn("ls", :pgroup => :true) should raise TypeError.
I updated specs with pgroup. Specs checks now that process executed in new group and also executed (print pid to STDOUT).

@LTe

Pass on travis but rake raise

/home/travis/builds/rubinius/rubinius/Rakefile:131:in `exit': no implicit conversion from nil to integer (TypeError)
2483    from /home/travis/builds/rubinius/rubinius/Rakefile:131:in `block in set_at_exit_handler'`

On my localhost:
3891 files, 21911 examples, 142349 expectations, 0 failures, 0 errors

@dbussink dbussink and 1 other commented on an outdated diff Sep 11, 2012
spec/ruby/shared/process/spawn.rb
@@ -268,6 +268,10 @@
it "raises an ArgumentError if given a negative :pgroup option" do
lambda { @object.spawn("echo", :pgroup => -1) }.should raise_error(ArgumentError)
end
+
+ it "does not raises an TypeError if given a true :pgroup option" do
@dbussink
dbussink Sep 11, 2012

Looks like this description is wrong. It raises an exception and the description says otherwise.

@LTe
LTe Sep 11, 2012

Good point!

@dbussink dbussink commented on an outdated diff Sep 11, 2012
kernel/common/process19.rb
@@ -73,7 +73,12 @@ def self.adjust_options(options)
when :in, :out, :err, Fixnum, IO
redirects[key] = adjust_redirect_value(key, value)
when :pgroup
- raise ArgumentError, "negative process group ID : #{value}" if value && value < 0
+ if value.kind_of?(Integer) && value < 0
+ raise ArgumentError, "negative process group ID : #{value}"
+ elsif value.kind_of?(Symbol)
+ raise TypeError, "can't convert Symbol into Integer"
@dbussink
dbussink Sep 11, 2012

It feels to me as what MRI does here is a standard Ruby coercion protocol to Integer. Looks like we need to do that too here, instead of just having a special case for Symbol.

@LTe

It feels to me as what MRI does here is a standard Ruby coercion protocol to Integer. Looks like we need to do that too here, instead of just having a special case for Symbol.

We need to consider several options

  • pgroup is true
  • pgroup is positive Integer
  • pgroup is negative Integer
  • pgroup is nil
  • pgroup is false

I moved a little bit of logic from this place to this place

Now when pgroup is true we set pgroup value to 0. Method coerce_to create once again 0 and set value. When pgroup is false or nil this value will set and default behavior will execute because of that. If pgroup will be Integer negative Rubinius will raise argument error because of that. If pgroup will be positive Integer everything will be normal.

@dbussink what do you think?

@dbussink dbussink and 1 other commented on an outdated diff Sep 12, 2012
kernel/common/process19.rb
@@ -73,6 +73,8 @@ def self.adjust_options(options)
when :in, :out, :err, Fixnum, IO
redirects[key] = adjust_redirect_value(key, value)
when :pgroup
+ value = 0 if value == true
+ value = Rubinius::Type.coerce_to value, Integer, :to_int if value
raise ArgumentError, "negative process group ID : #{value}" if value && value < 0
@dbussink
dbussink Sep 12, 2012

Looks like we have multiple if value checks here. What is the behavior when value is nil? We should try to only have to this check once probably.

@LTe
LTe Sep 12, 2012

When pgroup is nil or false process group will not create.

@LTe
LTe Sep 12, 2012

@dbussink we can do something like that:

value = 0 if value == true
if value
  value = Rubinius::Type.coerce_to value, Integer, :to_int 
  raise ArgumentError, "negative process group ID : #{value}" if value < 0
end
@dbussink
dbussink Sep 15, 2012

How about something like this?

if value == true
  value = 0
elsif value
  value = Rubinius::Type.coerce_to value, Integer, :to_int 
  raise ArgumentError, "negative process group ID : #{value}" if value < 0
end
LTe added some commits Sep 11, 2012
@LTe LTe Add spec for Process.spawn
When user execute method with :pgroup => true Rubinius tried to use '<'
method on TrueClass object. It will throw an exception NoMethotError.

Process.spawn executed with :pgroup => Symbol should raise TypeError
cdc63e1
@LTe LTe Change extract pgroup value
Cases:
1. pgroup value is 'true' => 0
2. pgroup value is kind of true => Rubinius will try coerce_to Integer
3. pgroup value if kind of true and pass coerce then Rubinius will check
check whether the value is < 0. If yes then an ArgumentError exception is thrown
4. pgroup if kind of false (false, nil) - default behavior (like without
pgroup)

Fixes #1894
848849d
@dbussink dbussink merged commit 5aea289 into rubinius:master Sep 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment