Skip to content
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

fiber Stack popMethod -> ArrayIndexOutOfBoundsException #309

Open
laughcat opened this issue Apr 19, 2018 · 7 comments
Open

fiber Stack popMethod -> ArrayIndexOutOfBoundsException #309

laughcat opened this issue Apr 19, 2018 · 7 comments

Comments

@laughcat
Copy link

laughcat commented Apr 19, 2018

I found a problem:it happens when i call the exchange method of RestTemplate,co.paralleluniverse.fibers.Stack.popMethod throws an ArrayIndexOutOfBoundsException at Stack.java:151, idx equals the length of dataLong.Could you tell me the reason?
image

image

@doctorpangloss
Copy link

doctorpangloss commented May 3, 2018

Increase the stack size in the fiber constructor.

@doctorpangloss
Copy link

@pron would it be a disaster if dataLong and dataObject (i.e., the actual stack) were just... ArrayLists? CopyOnWriteArrayLists?

@rustam9
Copy link

rustam9 commented Dec 5, 2018

@doctorpangloss , could you please elaborate? why would increasing the stack size help in this case? I thought the stack was supposed to grow during pushMethod if necessary, maybe we should call growStack in nextMethodEntry too when it bumps ps?

@doctorpangloss
Copy link

doctorpangloss commented Dec 6, 2018

@rustam9, that's a @pron question why the stack isn't growing in some situations. I've encountered this too, which is why I had the workaround.

I believe that it's possible to push twice but pop once or push once and miss a pop, which would explain why the stack pointer would be too large. I've seen this when examining the stack in a fiber (identical arguments on the stack appearing twice). My bet is that there's something with (1) default methods, (2) overridden methods calling super vs. this, (3) overloaded methods calling other overloads, or some overloads that override and some overloads that do not, especially ones that call each other, or (4) empty methods, especially empty methods that get overridden, especially empty methods in library code that get instrumented and are overridden more than once.

@rustam9
Copy link

rustam9 commented Dec 6, 2018

Thanks @doctorpangloss for the explanation! The workaround works for me as well.
I believe this has been reported at https://groups.google.com/forum/#!topic/quasar-pulsar-user/P6tObzUTLgU with index 96, here it's 48, and in my case it was 192.
It can't be just a coincidence that all problematic indices were exactly two folds of 48, and the default stack size is 32+1*16 = 48.
I tried to change the following logic in pushMethod:

diff --git a/quasar-core/src/main/java/co/paralleluniverse/fibers/Stack.java b/quasar-core/src/main/java/co/paralleluniverse/fibers/Stack.java
index 70c5821..9d9d1d0 100644
--- a/quasar-core/src/main/java/co/paralleluniverse/fibers/Stack.java
+++ b/quasar-core/src/main/java/co/paralleluniverse/fibers/Stack.java
@@ -131,7 +131,7 @@ public final class Stack implements Serializable {

         int nextMethodIdx = sp + numSlots;
         int nextMethodSP = nextMethodIdx + FRAME_RECORD_SIZE;
-        if (nextMethodSP > dataObject.length)
+        if (nextMethodSP >= dataObject.length)
             growStack(nextMethodSP);

         // clear next method's frame record

The ArrayIndexOutOfBoundsException disappeared without increasing the stack size for Fiber.
Even if it's not the right fix, I think it should be pretty close. I will see if I can try to reproduce this with a unit test.
@circlespainter @pron

@doctorpangloss
Copy link

doctorpangloss commented Dec 6, 2018

I think you hit the nail on the head there

@rockghost
Copy link

@doctorpangloss @pron @circlespainter Hi, my team is currently on heavy dev job based on Quasar and met the exactly same issue like this one. @rustam9 seems to find a workaround, right?

However could you make a patch or new release with this fix? Actually I know you are focusing project Loom but we don't have enough time for the release of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants