NPE in BiVariableMap.getLocalVarNames() after clear()

Details

Type: Bug
Status: Resolved
Priority: Blocker
Resolution: Fixed
Affects Version/s: JRuby 1.7.1, JRuby 1.7.2
Fix Version/s: JRuby 1.7.3
Component/s: Embedding
Labels:
None

Number of attachments :
0

Description

After calling ScriptingContainer.clear() (which should release global variables and mappings), I'm getting a NullPointerException upon a runScriptlet() call:

Caused by: java.lang.NullPointerException
	at org.jruby.embed.internal.BiVariableMap.getLocalVarNames(BiVariableMap.java:338)
	at org.jruby.embed.internal.EmbedRubyRuntimeAdapterImpl.getManyVarsDynamicScope(EmbedRubyRuntimeAdapterImpl.java:219)
	at org.jruby.embed.internal.EmbedRubyRuntimeAdapterImpl.runParser(EmbedRubyRuntimeAdapterImpl.java:172)
	at org.jruby.embed.internal.EmbedRubyRuntimeAdapterImpl.parse(EmbedRubyRuntimeAdapterImpl.java:92)
	at org.jruby.embed.ScriptingContainer.parse(ScriptingContainer.java:1195)
	at org.jruby.embed.ScriptingContainer.runScriptlet(ScriptingContainer.java:1255)
	at 	... 35 more

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Hiro Asari added a comment - 08/Jan/13 6:25 PM

Do you have a reproducible test case?

Show

Hiro Asari added a comment - 08/Jan/13 6:25 PM Do you have a reproducible test case?

Hide

Permalink

Gergely Nagy added a comment - 08/Jan/13 6:26 PM

more info: this did not occur with Jruby 1.5.6 at least.
Any workaround is welcome.
Thanks!

Show

Gergely Nagy added a comment - 08/Jan/13 6:26 PM more info: this did not occur with Jruby 1.5.6 at least. Any workaround is welcome. Thanks!

Hide

Permalink

Gergely Nagy added a comment - 10/Jan/13 10:16 AM - edited

> Do you have a reproducible test case?
Yes, I can do a pull request if you want but here it is:

package org.jruby.embed;
import static org.junit.Assert.*;

import org.junit.Before;
import org.junit.Test;

public class EmbedClearBug7058Test {
    private ScriptingContainer c;
    @Before
    public void setUp() {
        c = new ScriptingContainer(LocalContextScope.THREADSAFE);
    }
    @Test
    public void testDoesNotThrowNPEAfterClear_Bug7058() {
        c.clear();
        c.runScriptlet(""); // throws NPE
    }
    @Test
    public void testClearReleasesGlobalVariable_EndlessLoopIn1_7_2() {
        c.runScriptlet("$a = 1");
        assertEquals(1L, c.getVarMap().get("$a"));
        c.clear();
        assertEquals(null, c.getVarMap().get("$a"));
    }
    @Test
    public void testClearReleasesGlobalVariable_NPEIn1_7_2() {
        c.runScriptlet("$a = 1");
        assertEquals(1L, c.runScriptlet("$a"));
        assertEquals(1L, c.getVarMap().get("$a"));
        c.clear();
        assertEquals(null, c.runScriptlet("$a"));
    }
    @Test
    public void testClearReleasesGlobalVariable_FailsIn1_6_7_NPEIn1_7_2() {
        c.runScriptlet("$a = 1");
        assertEquals(1L, c.runScriptlet("$a"));
        c.clear();
        assertEquals(null, c.runScriptlet("$a"));
    }

}

Show

Gergely Nagy added a comment - 10/Jan/13 10:16 AM - edited > Do you have a reproducible test case? Yes, I can do a pull request if you want but here it is: package org.jruby.embed; import static org.junit.Assert.*; import org.junit.Before; import org.junit.Test; public class EmbedClearBug7058Test { private ScriptingContainer c; @Before public void setUp() { c = new ScriptingContainer(LocalContextScope.THREADSAFE); } @Test public void testDoesNotThrowNPEAfterClear_Bug7058() { c.clear(); c.runScriptlet(""); // throws NPE } @Test public void testClearReleasesGlobalVariable_EndlessLoopIn1_7_2() { c.runScriptlet( "$a = 1" ); assertEquals(1L, c.getVarMap().get( "$a" )); c.clear(); assertEquals( null , c.getVarMap().get( "$a" )); } @Test public void testClearReleasesGlobalVariable_NPEIn1_7_2() { c.runScriptlet( "$a = 1" ); assertEquals(1L, c.runScriptlet( "$a" )); assertEquals(1L, c.getVarMap().get( "$a" )); c.clear(); assertEquals( null , c.runScriptlet( "$a" )); } @Test public void testClearReleasesGlobalVariable_FailsIn1_6_7_NPEIn1_7_2() { c.runScriptlet( "$a = 1" ); assertEquals(1L, c.runScriptlet( "$a" )); c.clear(); assertEquals( null , c.runScriptlet( "$a" )); } }

Hide

Permalink

Gergely Nagy added a comment - 13/Jan/13 5:24 AM

I just need to ensure that global variables are reset/cleared between script runs.
That exception is blocking my upgrade from 1.5.6 to 1.7.2 - so any easy workaround would be welcome.
Thanks.

Show

Gergely Nagy added a comment - 13/Jan/13 5:24 AM I just need to ensure that global variables are reset/cleared between script runs. That exception is blocking my upgrade from 1.5.6 to 1.7.2 - so any easy workaround would be welcome. Thanks.

Hide

Permalink

Gergely Nagy added a comment - 24/Jan/13 8:39 PM

Added pull request for testcases https://github.com/jruby/jruby/pull/508

Show

Gergely Nagy added a comment - 24/Jan/13 8:39 PM Added pull request for testcases https://github.com/jruby/jruby/pull/508

Hide

Permalink

Mark Richards added a comment - 29/Jan/13 9:58 AM

I can verify that this is caused by a change to BiVariableMap.clear() after 1.7.0. The clear() method adds a null object to the variables member if you don't have a variable called "ARGV". After this happens, subsequent calls to jruby result in an endless loop later on in getVariable.

I changed the clear() method as follows:

line 472

variables.add(argv_object);
+ if (argv_object != null) variables.add(argv_object);

and that fixed it for me.

Show

Mark Richards added a comment - 29/Jan/13 9:58 AM I can verify that this is caused by a change to BiVariableMap.clear() after 1.7.0. The clear() method adds a null object to the variables member if you don't have a variable called "ARGV". After this happens, subsequent calls to jruby result in an endless loop later on in getVariable. I changed the clear() method as follows: line 472 variables.add(argv_object); + if (argv_object != null) variables.add(argv_object); and that fixed it for me.

Hide

Permalink

Gergely Nagy added a comment - 30/Jan/13 10:50 AM

@Mark, yeah, thanks.
Does this pass tests above?
I was also looking at the code, and saw fishy things around there - but I would really let the maintainers to come up with a proper solution. But if you can commit or provide a pull request for them that would be great.
This is still blocking my upgrade (and I just missed a window of opportunity there), I'm interested to speed this up. At least some attention would be nice. Thanks again!

Show

Gergely Nagy added a comment - 30/Jan/13 10:50 AM @Mark, yeah, thanks. Does this pass tests above? I was also looking at the code, and saw fishy things around there - but I would really let the maintainers to come up with a proper solution. But if you can commit or provide a pull request for them that would be great. This is still blocking my upgrade (and I just missed a window of opportunity there), I'm interested to speed this up. At least some attention would be nice. Thanks again!

Hide

Permalink

Yoko Harada added a comment - 10/Feb/13 8:10 PM

Hi,

Sorry for taking so long to fix this bug. Finally, I pushed the fix whose revision is 41ae762 .

Basically, @Mark is right. That add method added null without checking anything. That change fixed @Gergely's test cases.

More than that, creating arrays while instantiating the BiVariableMap class was not a good idea. It turned out BiVariableMap had 10 nulls in arrays. I fixed as well.

I also added @Gergely's test case to BiVariableMapTest so that it won't regress.

Thanks for reporting this and patiently waiting the fix.

Show

Yoko Harada added a comment - 10/Feb/13 8:10 PM Hi, Sorry for taking so long to fix this bug. Finally, I pushed the fix whose revision is 41ae762 . Basically, @Mark is right. That add method added null without checking anything. That change fixed @Gergely's test cases. More than that, creating arrays while instantiating the BiVariableMap class was not a good idea. It turned out BiVariableMap had 10 nulls in arrays. I fixed as well. I also added @Gergely's test case to BiVariableMapTest so that it won't regress. Thanks for reporting this and patiently waiting the fix.

Hide

Permalink

Yoko Harada added a comment - 10/Feb/13 8:13 PM

The bug has been fixed.

If there's further problem, please file a new issue.
Or, reopening this issue would be an option if the bug is the same as this.

Show

Yoko Harada added a comment - 10/Feb/13 8:13 PM The bug has been fixed. If there's further problem, please file a new issue. Or, reopening this issue would be an option if the bug is the same as this.

People

Assignee:

Yoko Harada

Reporter:

Gergely Nagy

Vote (0)

Watch (5)

Dates

Created:

08/Jan/13 6:23 PM

Updated:

10/Feb/13 8:13 PM

Resolved:

10/Feb/13 8:13 PM