Issue2559

When running the regression tests on Ubuntu 16.04 with Java version 1.8.0_121 the test_marshal test fails. The error is below.

[exec] test test_marshal failed -- multiple errors occurred; run in verbose mode for details

The cause of the failure is a StackOverflowError this can be fixed by setting the ThreadStackSize for the JVM larger. I found 3m to be enough so pass -Xss3m to the JVM and the test passes.

As this is a test specifically for this i'm not clear that increasing the ThreadStackSize is the best solution or how that option should be added to the tests.

Maybe this should be resolved for 2.7.1?

Just a bit more info on testing this

james@james-ubuntu:~/git/jython$ java -jar dist/jython.jar dist/Lib/test/test_marshal.py -v
test_bool (__main__.IntTestCase) ... ok
test_int64 (__main__.IntTestCase) ... ok
test_ints (__main__.IntTestCase) ... ok
test_floats (__main__.FloatTestCase) ... ok
test_string (__main__.StringTestCase) ... ok
test_unicode (__main__.StringTestCase) ... ok
test_dict (__main__.ContainerTestCase) ... ok
test_list (__main__.ContainerTestCase) ... ok
test_sets (__main__.ContainerTestCase) ... ok
test_tuple (__main__.ContainerTestCase) ... ok
test_exceptions (__main__.ExceptionTestCase) ... ok
test_bug_5888452 (__main__.BugsTestCase) ... ok
test_fuzz (__main__.BugsTestCase) ... ok
test_loads_recursion (__main__.BugsTestCase) ... ERROR
test_patch_873224 (__main__.BugsTestCase) ... ok
test_recursion_limit (__main__.BugsTestCase) ... ERROR
test_version_argument (__main__.BugsTestCase) ... ok

======================================================================
ERROR: test_loads_recursion (__main__.BugsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "dist/Lib/test/test_marshal.py", line 232, in test_loads_recursion
    self.assertRaises(ValueError, marshal.loads, s)
  File "/home/james/git/jython/dist/Lib/unittest/case.py", line 476, in assertRaises
    callableObj(*args, **kwargs)
  File "/home/james/git/jython/dist/Lib/marshal.py", line 28, in loads
    return load(f)
  File "/home/james/git/jython/dist/Lib/marshal.py", line 19, in load
    return u.load()
RuntimeError: maximum recursion depth exceeded (Java StackOverflowError)

======================================================================
ERROR: test_recursion_limit (__main__.BugsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "dist/Lib/test/test_marshal.py", line 244, in test_recursion_limit
    data = marshal.dumps(head)
  File "/home/james/git/jython/dist/Lib/marshal.py", line 23, in dumps
    dump(x, f, version)
  File "/home/james/git/jython/dist/Lib/marshal.py", line 12, in dump
    Marshaller(f, version).dump(x)
RuntimeError: maximum recursion depth exceeded (Java StackOverflowError)

----------------------------------------------------------------------
Ran 17 tests in 0.335s

FAILED (errors=2)
Traceback (most recent call last):
  File "dist/Lib/test/test_marshal.py", line 264, in <module>
    test_main()
  File "dist/Lib/test/test_marshal.py", line 255, in test_main
    test_support.run_unittest(IntTestCase,
  File "/home/james/git/jython/dist/Lib/test/test_support.py", line 1320, in run_unittest
    _run_suite(suite)
  File "/home/james/git/jython/dist/Lib/test/test_support.py", line 1303, in _run_suite
    raise TestFailed(err)
test.test_support.TestFailed: multiple errors occurred
james@james-ubuntu:~/git/jython$ 


But with -Xss3m

james@james-ubuntu:~/git/jython$ java -Xss3m -jar dist/jython.jar dist/Lib/test/test_marshal.py -v
test_bool (__main__.IntTestCase) ... ok
test_int64 (__main__.IntTestCase) ... ok
test_ints (__main__.IntTestCase) ... ok
test_floats (__main__.FloatTestCase) ... ok
test_string (__main__.StringTestCase) ... ok
test_unicode (__main__.StringTestCase) ... ok
test_dict (__main__.ContainerTestCase) ... ok
test_list (__main__.ContainerTestCase) ... ok
test_sets (__main__.ContainerTestCase) ... ok
test_tuple (__main__.ContainerTestCase) ... ok
test_exceptions (__main__.ExceptionTestCase) ... ok
test_bug_5888452 (__main__.BugsTestCase) ... ok
test_fuzz (__main__.BugsTestCase) ... ok
test_loads_recursion (__main__.BugsTestCase) ... ok
test_patch_873224 (__main__.BugsTestCase) ... ok
test_recursion_limit (__main__.BugsTestCase) ... ok
test_version_argument (__main__.BugsTestCase) ... ok

----------------------------------------------------------------------
Ran 17 tests in 0.305s

OK
james@james-ubuntu:~/git/jython$ 

If the point of the test is to check for infinite recurrsion then increasing Xss should be ok if the point is to check the stack doesn't get to big then maybe its not a good solution.

A few Jython launch scripts specify -Xss1m so it was considered at some point...

Sounds like the best alternative is to increase the stack size, given that this is a recursion test after all.

Presumably what we are seeing differences in how much stack allocations cost between different OSes, architectures, and Java implementations. There's no standard here that we can rely on that states exactly how much stack allocation some set of ops will cost for Java, so we have to be conservative and ask for more than we might expect.

I have a pull request to fix this https://github.com/jythontools/jython/pull/61

It sets a Xss significantly higher than required so it should be sufficient in all cases.

James: Your fix in https://github.com/jythontools/jython/pull/61 only addresses Linux et al. The issue would persist on Windows.
jython.exe is compiled from jython.py, so an equivalent modification in jython.py would be needed.

However, I am not sure if this ought to be fixed in the launcher. Do we want Jython to start with such an increased stacksize always? Just because one regrtest depends on it..? Maybe we should rather adjust regrtest target in build.xml.

Or as a compromise we could set the default launcher to 3m and adjust build.xml to run tests with 5m.

There is no need to fix this change in the launcher, given that this is just for a specific test at the regrtest level.

Users can also specify the desired stack size they need, via -J options; or by directly using Java (eg java -jar jython.jar...).

I would argue:

1. If we don't expect the test to pass under standard conditions as written, then we should fix the test.

2. If we believe the test demands no more than users can legitimately expect from Jython, then we should fix the behaviour (on all platforms).

I tend to think 2 applies in this case. A particular expectation (2000) is acknowledged in _marshal.java, and I think the test should match it (roughly as the comment says, but not marshal.c).

This looks relevant: http://bugs.python.org/issue22734, but I suspect the motivation is specific to C-on-Windows, not Windows per se. With Java I think we can expect uniformity cross-platform.

Thanks for the comments, I completely forgot to test this on Windows and yes that needs to be sorted out. I tried changing the Xss setting in the python launch script but that didn't work on windows the JVM still had Xss=1024k. Looking a bit more at this I realise this is because the jython.exe is actually built from the jython.py script somehow I couldn't figure out how, maybe using PyInstaller? Does that need to be done on be Windows? I would be happy to do this if someone can give me some hints to the process. Initially I though the exe just called the script.

On the other points, I think this should be changed in the launchers (Unix + Windows). If people run Jython by clicking the exe I think should run in a state where the unit tests could pass. Additionally running the regrtest's on Windows using the jython_regrtest.bat script relies on the launcher. So it need to work there, if the unit tests on Windows are to pass (at least if this script is used).

Additionally i'm not sure what the advantage to fixing it in the regrtest, but not in the launcher would be? We would save 2mb of ram?

Finally what value of Xss would be the best to use? In the pull request I changed to 5m but after a bit more thought I think this is probably more than enough, and would now propose to change to 3m. I know this is a bit arbitrary (pick a number >3) but It would seem reasonable to keep it as small as needed for the tests to pass on every platform (that we can reasonably test).

We use PyInstaller; see http://bugs.jython.org/issue1491 for the details on how this was done. It should be automated although IIRC this requires building on Windows.

The default stack size is set by the JAVA_STACK env var, which in turn defaults to -Xss1024k

Re stack size: this is for each thread, so it can add up. However, we no longer use the default for heap, since it's too low for Jython; it's set to 512M; we may want to increase stack size by default as well to somewhat higher.

See https://github.com/jythontools/jython/blob/master/src/shell/jython.py#L211

As follows we can retrieve the Xss value at runtime.

Jython 2.7.1rc1 (, Mär 4 2017, 22:31:12) 
[OpenJDK 64-Bit Server VM (Oracle Corporation)] on java1.8.0_121
Type "help", "copyright", "credits" or "license" for more information.
>>> from java.lang.management import ManagementFactory
>>> rtmx = ManagementFactory.getRuntimeMXBean()
>>> rtmx
sun.management.RuntimeImpl@64f555e7
>>> ags = rtmx.getInputArguments()
>>> ags
[-Xmx512m, -Xss1024k, -Dpython.home=dist, -Dpython.executable=dist/bin/jython]
>>> ags[1]
u'-Xss1024k'
>>> 

Based on this we could implement a @skipif guarding the case that Xss is <3m.

Parsing of the -Xss value should better done on Java-side in a utility method, e.g. in Py.java or so. Also one must consider to properly map the k, m or whatever properly.

I know, this method only reveals -Xss if that arg was given on command line. However since our launchers always provide it, this would work reliably on Jython. Should still have a fallback (to None?) if the value wasn't provided, e.g. because launched directly via java-command.

How should None then be treated by the @skipif? Actually I don't care. Either way is fine; someone running tests not using a launcher should know what he is doing.

I have updated the pull request to fix this https://github.com/jythontools/jython/pull/61

It now sets Xss to 2560k on both Unix and Windows. I think this is a good compromise, on Ubuntu test_marshal fails with 2m but passes with 3m. It actually needs some where around 2100k so 2560k seems like a reasonable value while staying as small as possible.

I have rebuilt the windows launcher and although it seems to work for me I would be happier if a few others could try this out as well.

Have just updated the pull request again so now the test is updated as well

Committed at https://hg.python.org/jython/rev/17e40de9a541