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

Java type in karate-config with parallel runner causing error: Multi threaded access #1558

Closed
ericdriggs opened this issue Apr 14, 2021 · 24 comments
Assignees

Comments

@ericdriggs
Copy link

Given using config = karate.callSingle to define a Java.type variable
When execute parallel runner,
Then get reliable "Multi threaded access requested by thread" error

MVC repo (tests pass when run individually, failure when run in parallel):
https://github.com/ericdriggs/karate-parallel-runner-concurrent-access/

Seems related to oracle/graal#631

@ericdriggs ericdriggs changed the title Java type in karate-config causing Multi threaded access with parallel runner Java type in karate-config with parallel runner causing error: Multi threaded access Apr 14, 2021
@ericdriggs
Copy link
Author

ericdriggs commented Apr 14, 2021

Think I can probably do a workaround by defining a javascript object and then defining individual functions on that javascript object to simulate static method interop, only doing Java.type inside the function call
Still, an interesting race condition, and a crying shame that graal has such a ridiculous default without a flag to turn it off.

@ptrthomas ptrthomas self-assigned this Apr 15, 2021
@ptrthomas ptrthomas added the bug label Apr 15, 2021
@ptrthomas ptrthomas added this to the 1.0.2 milestone Apr 15, 2021
@ptrthomas
Copy link
Member

thanks, this helps cc @joelpramos

@ptrthomas ptrthomas added wontfix and removed bug labels Apr 16, 2021
@ptrthomas ptrthomas removed this from the 1.0.2 milestone Apr 16, 2021
@ptrthomas
Copy link
Member

ptrthomas commented Apr 16, 2021

@ericdriggs so - this is a won't fix. I've added this below to the documentation. given the way graal js behaves today there is no way we can get a java instance created on one thread to work across threads. anybody is welcome to find a way.

I've tweaked our existing parallel troublemaker test to replicate this if you comment out one line in the offending karate-config.js.

image

@joelpramos
Copy link
Contributor

joelpramos commented Apr 18, 2021

@ptrthomas far from me to say this solves everything (cause you know, Graal) but either of lines added in the catch in the screenshot below solves the specific reproduce unit test you added (either this.JS.context.asValue(value) or the commented line Value.asValue(value.asHostObject()), both work).

I think it's worth adding Value.asValue(value.asHostObject()) (avoids access to JS.context) on the catch and/or slightly modify the logic / catch method (and warn message).

image

@ptrthomas ptrthomas added bug and removed wontfix labels Apr 18, 2021
@ptrthomas ptrthomas added this to the 1.0.2 milestone Apr 18, 2021
@ptrthomas
Copy link
Member

@joelpramos that's a neat trick and I'm still not sure how it works, and glad I had the tests lined up. do review !

@ericdriggs
Copy link
Author

ericdriggs commented May 17, 2021

@ptrthomas
Still seeing this issue when using karate 1.1.0-RC1

ericdriggs/karate-parallel-runner-concurrent-access@81003b0

mvn clean test

...
[ERROR] Failures: 
[ERROR]   ParallelRunnerTest.testParallel:18 classpath:examples/parallel-runner-always-fails.feature:9
karate-config.js
Multi threaded access requested by thread Thread[pool-1-thread-1,5,main] but is not allowed for language(s) js.
.....

@ptrthomas
Copy link
Member

not planning to look at this immediately. any help welcome

@ptrthomas ptrthomas removed the fixed label May 18, 2021
ptrthomas added a commit that referenced this issue May 23, 2021
work in progress and greatly improved error logging for config js failures
ptrthomas added a commit that referenced this issue May 23, 2021
@ptrthomas
Copy link
Member

@joelpramos the second point (init()) is no longer going to lock with the latest changes as far as I can tell - but you can confirm

the rest - you are welcome to prove there is a significant impact (I think not, all the processing is in-memory, and based on the build run-time) and make changes. this has not been easy to work on

@joelpramos
Copy link
Contributor

Like you I'm not too fussed about it. The one on the init() raised more one of my eyebrows. I think the other logging / naming comments are still valid.

Hopefully the battle is over.

ptrthomas added a commit that referenced this issue Jun 19, 2021
this time simlifying the detach routine to be less prone to graal issues
not able to avoid the synchronize for recurseAndAttach()
had to add a coupld more but thankfully only for the callonce / callsingle flows
but still saw a failure once in a while
@ptrthomas
Copy link
Member

@joelpramos tried to remove the sync on recurseAndAttach() but couldn't. so over to you now :) I'm done for today

tried a few more things as you can see in the prev commit

a tip: running this can replicate the problem easily on local

mvn test -f karate-core/pom.xml -Dtest=Parallel*Test

ptrthomas added a commit that referenced this issue Jun 19, 2021
no more locking at all except for the callonce / callSingle flows
if any troublesome value is encountered we throw it out with a log message and the name of the variable
if tests depend on functions from callonce / callSingle or java classes / code they may fail
but thats the fault of the user - the log messages should guide you what to stop doing
and if you switch on TRACE logging you will see more detailed stack traces for diagnosis
ptrthomas added a commit that referenced this issue Jun 21, 2021
ptrthomas added a commit that referenced this issue Jun 21, 2021
ptrthomas added a commit that referenced this issue Jun 21, 2021
ptrthomas added a commit that referenced this issue Jun 21, 2021
@ericdriggs
Copy link
Author

Thanks for your continued work on this.

@joelpramos
Copy link
Contributor

Thanks for your continued work on this.

indeed!

@ptrthomas I added a few comments in the commits but it's mostly to wrap my head around the changes you are doing so that when I have some time to fetch the code and iterate on this I'm not totally lost

@ptrthomas
Copy link
Member

just released 1.1.0.RC4 - I request everyone to re-test this once cc @aleruz

@ericdriggs
Copy link
Author

Pass - Validated on RC4 for MVC project and my own projects.

ptrthomas added a commit that referenced this issue Jul 1, 2021
ptrthomas added a commit that referenced this issue Jul 6, 2021
which is to pass java functions around as Function instances not JS snippets
which means they become graal host objects - which do not have the multi-threading issues
added some docs to make this clear etc
@ptrthomas
Copy link
Member

all please see this comment on the other related issue. it is possible to use java functions (not just classes) safely and work around some of the graal limitations: #1633 (comment)

@ericdriggs
Copy link
Author

@ptrthomas
My sample project is still passing unchanged on both RC4 and a local build of the latest develop.
Can you please clarify if the function recommendation is recommended means of java interop in callOnce? The additional boilerplate isn't pretty since it requires duplicate interface at both the class and js level but if that's what's required I understand.

@ptrthomas
Copy link
Member

ptrthomas commented Jul 8, 2021

@ericdriggs reco is only if you

a) use java functions ("use" means you actually call the function in parallel scenarios)
b) and callonce or callsingle
c) you really want to "save some typing" by referring to functions instead of passing a Java.type() around and using that at point-of-need

I also feel the extra code is only in the Java side. the JS side is the same one or two lines

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

No branches or pull requests

3 participants