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

StackOverflow on using method_added in a mixin #4839

Open
flash-gordon opened this Issue Nov 4, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@flash-gordon

flash-gordon commented Nov 4, 2017

Environment

jruby -v
jruby 9.1.13.0 (2.3.3) 2017-09-06 8e1c115 Java HotSpot(TM) 64-Bit Server VM 25.121-b13 on 1.8.0_121-b13 +jit [darwin-x86_64]
Darwin MacBook-Pro-2.local 17.0.0 Darwin Kernel Version 17.0.0: Thu Aug 24 21:48:19 PDT 2017; root:xnu-4570.1.46~2/RELEASE_X86_64 x86_64

Expected Behavior

In https://github.com/dry-rb/dry-validation/ we have some dynamic behavior involving using the method_added hook that leads to an error on jruby. The error is minor and I can easily work around it. The minimal code for reproducing the issue follows:

class A
end

class B < A
end

module C
  def method_added(name)
    super
  end
end

B.extend(C)
A.extend(C)
# ^Note the order in which classes are extended

class B
  def foo
  end
end

MRI successfully executes this piece of code whereas jruby fails with the following error:

Error: Your application used more stack memory than the safety cap of 2048K.
Specify -J-Xss####k to increase it (#### = cap size in KB).
Specify -w for full java.lang.StackOverflowError stack trace

@headius headius added this to the JRuby 9.1.15.0 milestone Nov 9, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 9, 2017

Member

Thanks for the reproduction, shouldn't be hard to find the issue.

Member

headius commented Nov 9, 2017

Thanks for the reproduction, shouldn't be hard to find the issue.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 9, 2017

Member

Confirmed on master (9.2). I assume 9.1.14 still has the issue.

Member

headius commented Nov 9, 2017

Confirmed on master (9.2). I assume 9.1.14 still has the issue.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 5, 2017

Member

Ok, so I looked into this a bit.

The problem here is how we resolve the super call.

This is a bit of an unusual situation because normally you would not have two of the same module in a class hierarchy; includes against a hierarchy that already has a module will not re-add that module. The order here forces the problem.

Our class hierarchies match MRI, with the "A" hierarchy having a single "C" module and the "B" hierarchy having two "C" modules. And in MRI, the custom method_added does indeed get called twice.

In JRuby, however, the super here is compiled as "unresolved" which means we inspect the class hierarchy to figure out where we're "supering" from. The logic for this currently searches up the hierarchy for the method's "implementation class" (which would be the "C" module here), and unsurprisingly that always resets the super to the lower of the two "C" modules in "B"'s hierarchy.

Trouble is, I'm not sure the best way to fix it. In MRI, they resolve this by having method caches also keep the pseudo-location within the hierarchy, so the super always knows where to start. We do not have that information in our caches, so of course none of our call site logic is set up to use it.

Since you say the error is minor, I'm inclined to bump this to a major release rather than start mucking about with method caches on our maintenance line. But it should be fixed.

Workaround for anyone following along: make sure you're never including (or extending) a module into a descendant of a hierarchy before you include or extend it in a an ancestor.

Member

headius commented Dec 5, 2017

Ok, so I looked into this a bit.

The problem here is how we resolve the super call.

This is a bit of an unusual situation because normally you would not have two of the same module in a class hierarchy; includes against a hierarchy that already has a module will not re-add that module. The order here forces the problem.

Our class hierarchies match MRI, with the "A" hierarchy having a single "C" module and the "B" hierarchy having two "C" modules. And in MRI, the custom method_added does indeed get called twice.

In JRuby, however, the super here is compiled as "unresolved" which means we inspect the class hierarchy to figure out where we're "supering" from. The logic for this currently searches up the hierarchy for the method's "implementation class" (which would be the "C" module here), and unsurprisingly that always resets the super to the lower of the two "C" modules in "B"'s hierarchy.

Trouble is, I'm not sure the best way to fix it. In MRI, they resolve this by having method caches also keep the pseudo-location within the hierarchy, so the super always knows where to start. We do not have that information in our caches, so of course none of our call site logic is set up to use it.

Since you say the error is minor, I'm inclined to bump this to a major release rather than start mucking about with method caches on our maintenance line. But it should be fixed.

Workaround for anyone following along: make sure you're never including (or extending) a module into a descendant of a hierarchy before you include or extend it in a an ancestor.

@headius headius modified the milestones: JRuby 9.1.15.0, JRuby 9.2.0.0 Dec 5, 2017

@flash-gordon

This comment has been minimized.

Show comment
Hide comment
@flash-gordon

flash-gordon Dec 7, 2017

@headius many thanks for the explanation, it's not a problem for me whatsoever. Quite the opposite, there was no intention to write code that works like this :)

flash-gordon commented Dec 7, 2017

@headius many thanks for the explanation, it's not a problem for me whatsoever. Quite the opposite, there was no intention to write code that works like this :)

@aamarill

This comment has been minimized.

Show comment
Hide comment
@aamarill

aamarill May 14, 2018

Hello, I am looking for my first open source contribution, is this issue still up for grabs?

aamarill commented May 14, 2018

Hello, I am looking for my first open source contribution, is this issue still up for grabs?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 14, 2018

Member

@aamarill Yes, it is! Since we have a fairly simple reproduction, I'd recommend looking at the overflowed stack trace and starting from there. The problem is likely that super is finding the wrong location for the next level of class, and so it just keeps going back to the bottom of the hierarchy.

Member

headius commented May 14, 2018

@aamarill Yes, it is! Since we have a fairly simple reproduction, I'd recommend looking at the overflowed stack trace and starting from there. The problem is likely that super is finding the wrong location for the next level of class, and so it just keeps going back to the bottom of the hierarchy.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 14, 2018

Member

@aamarill I made some improvements to how super works on the "super_fixes" branch. You should try building it and see whether it fixes this issue. I will update it to current master.

Member

headius commented May 14, 2018

@aamarill I made some improvements to how super works on the "super_fixes" branch. You should try building it and see whether it fixes this issue. I will update it to current master.

@aamarill

This comment has been minimized.

Show comment
Hide comment
@aamarill

aamarill May 14, 2018

@headius Thank you for the explanation :) I should be able to start working on this today

aamarill commented May 14, 2018

@headius Thank you for the explanation :) I should be able to start working on this today

@enebo enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018

@jsyeo

This comment has been minimized.

Show comment
Hide comment
@jsyeo

jsyeo Sep 20, 2018

Contributor

You should try building it and see whether it fixes this issue. I will update it to current master.

I just compiled and tested with that branch. It works. 😉

Contributor

jsyeo commented Sep 20, 2018

You should try building it and see whether it fixes this issue. I will update it to current master.

I just compiled and tested with that branch. It works. 😉

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 21, 2018

Member

I had almost forgotten about this branch :-) I'll rebase it and see how it looks with a few month's worth of other changes

Member

headius commented Sep 21, 2018

I had almost forgotten about this branch :-) I'll rebase it and see how it looks with a few month's worth of other changes

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