Exception dumping depends on MRI-specific internals #103

Open
headius opened this Issue Nov 17, 2012 · 7 comments

Comments

Projects
None yet
3 participants
Contributor

headius commented Nov 17, 2012

The following method depends on MRI-specific internal variables in Exception objects, and as a result it doesn't marshal exceptions properly in JRuby:

      def visit_Exception o
        tag = ['!ruby/exception', o.class.name].join ':'

        @emitter.start_mapping nil, tag, false, Nodes::Mapping::BLOCK

        {
          'message'   => private_iv_get(o, 'mesg'),
          'backtrace' => private_iv_get(o, 'backtrace'),
        }.each do |k,v|
          next unless v
          @emitter.scalar k, nil, nil, true, false, Nodes::Scalar::ANY
          accept v
        end

        dump_ivars o

        @emitter.end_mapping
      end

I'll see if I can come up with a patch.

Contributor

headius commented Nov 17, 2012

Patch...this lets test_yaml_tree.rb pass (test_exception was failing with the original code).

diff --git a/lib/ruby/1.9/psych/visitors/yaml_tree.rb b/lib/ruby/1.9/psych/visitors/yaml_tree.rb
index d420abd..3f615f3 100644
--- a/lib/ruby/1.9/psych/visitors/yaml_tree.rb
+++ b/lib/ruby/1.9/psych/visitors/yaml_tree.rb
@@ -147,8 +147,8 @@ module Psych
         @emitter.start_mapping nil, tag, false, Nodes::Mapping::BLOCK

         {
-          'message'   => private_iv_get(o, 'mesg'),
-          'backtrace' => private_iv_get(o, 'backtrace'),
+          'message'   => private_iv_get(o, 'mesg') || o.message,
+          'backtrace' => private_iv_get(o, 'backtrace' || o.backtrace),
         }.each do |k,v|
           next unless v
           @emitter.scalar k, nil, nil, true, false, Nodes::Scalar::ANY
Contributor

headius commented Nov 17, 2012

private_iv_get is pretty gross, btw, but so is MRI's use of non-ivar ivars :-)

Fryguy commented Nov 17, 2012

@headius, from my use cases, I'm not sure Psych is dumping/loading Exception objects properly for MRI either. See issues #85 and #86 for things I've run into.

Owner

tenderlove commented Jan 12, 2013

@headius Are you sure this patch is correct? The line in the patch that calls o.backtrace seems broken.

Contributor

headius commented Jan 16, 2013

Updated patch, but we're still discussing...

diff --git a/lib/psych/visitors/yaml_tree.rb b/lib/psych/visitors/yaml_tree.rb
index ce40a17..1d01310 100644
--- a/lib/psych/visitors/yaml_tree.rb
+++ b/lib/psych/visitors/yaml_tree.rb
@@ -147,8 +147,8 @@ module Psych
         @emitter.start_mapping nil, tag, false, Nodes::Mapping::BLOCK

         {
-          'message'   => private_iv_get(o, 'mesg'),
-          'backtrace' => private_iv_get(o, 'backtrace'),
+          'message'   => private_iv_get(o, 'mesg') || o.message,
+          'backtrace' => private_iv_get(o, 'backtrace') || o.backtrace,
         }.each do |k,v|
           next unless v
           @emitter.scalar k, nil, nil, true, false, Nodes::Scalar::ANY
Owner

tenderlove commented Jan 16, 2013

@headius how about this:

diff --git a/src/org/jruby/ext/psych/PsychYamlTree.java b/src/org/jruby/ext/psych/PsychYamlTree.java
index 7b2c72e..515a0ef 100644
--- a/src/org/jruby/ext/psych/PsychYamlTree.java
+++ b/src/org/jruby/ext/psych/PsychYamlTree.java
@@ -47,9 +47,6 @@ public class PsychYamlTree {

     @JRubyMethod(visibility = PRIVATE)
     public static IRubyObject private_iv_get(ThreadContext context, IRubyObject self, IRubyObject target, IRubyObject prop) {
-        IRubyObject obj = target.getInstanceVariables().getInstanceVariable(prop.asJavaString());
-        if (obj == null) obj = context.nil;
-
-        return obj;
+        return target.callMethod(context, prop.asJavaString());
     }
 }
Contributor

headius commented Jan 16, 2013

But the method to get the internal variable "mesg" is "message". This won't fix that case.

It also seems a little special-casey. private_iv_get is doing what it's supposed to, but we don't store exception data in those variables.

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