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

JS / Multi threaded access error when using karate.callSingle() #1515

Closed
JoshSchreuder opened this issue Mar 15, 2021 · 60 comments
Closed

JS / Multi threaded access error when using karate.callSingle() #1515

JoshSchreuder opened this issue Mar 15, 2021 · 60 comments
Assignees

Comments

@JoshSchreuder
Copy link

JoshSchreuder commented Mar 15, 2021

I mentioned this issue here:
#1373 (comment)

Thought it might be fixed in #1480 in 1.0 but it appears not having tried the release version this morning.

Here is a minimal repro repo:
https://github.com/JoshSchreuder/karate-repro

0.9.6
java "-Dkarate.config.dir=C:\Temp\karaterepro" -jar "C:\temp\karate0.9.6.jar" "C:\Temp\karaterepro\demo\api"
is working with no issues

1.0.0
java "-Dkarate.config.dir=C:\Temp\karaterepro" -jar "C:\temp\karate1.0.jar" "C:\Temp\karaterepro\demo\api"
fails with

org.graalvm.polyglot.PolyglotException: java.io.FileNotFoundException: C:\Temp\karaterepro\demo\api\C:\Temp\karaterepro\features\auth\auth.feature (The filename, directory name, or volume label syntax is incorrect)
- com.intuit.karate.resource.FileResource.getStream(FileResource.java:98)
- com.intuit.karate.core.FeatureParser.parse(FeatureParser.java:73)
- com.intuit.karate.core.Feature.read(Feature.java:67)
- com.intuit.karate.core.ScenarioFileReader.readFile(ScenarioFileReader.java:64)
- com.intuit.karate.core.ScenarioBridge.read(ScenarioBridge.java:625)
- com.intuit.karate.core.ScenarioBridge.callSingle(ScenarioBridge.java:209)
- com.intuit.karate.core.ScenarioBridge.callSingle(ScenarioBridge.java:155)

It looks like it's appending the test directory to the start of the callSingle call? Couldn't find anything in the breaking changes about this, so not sure if it's intentional or a bug.

I'm on Windows 10 x64 21H1.
Java:

openjdk 11.0.10 2021-01-19
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.10+9)
Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.24.0, JRE 11 Windows 10 amd64-64-Bit Compressed References 20210120_899 (JIT enabled, AOT enabled)
OpenJ9   - 345e1b09e
OMR      - 741e94ea8
JCL      - 0a86953833 based on jdk-11.0.10+9)
@JoshSchreuder JoshSchreuder changed the title Cannot find file in callSingle (1.0) FileNotFoundException in callSingle (1.0) Mar 15, 2021
@JoshSchreuder JoshSchreuder changed the title FileNotFoundException in callSingle (1.0) FileNotFoundException in callSingle inside karate.config.js (1.0) Mar 15, 2021
@ptrthomas
Copy link
Member

@JoshSchreuder well, it would have been nice if you tested the last RC as requested. I'm going to wait on this as windows is difficult for me to troubleshoot :| anyone reading this is welcome to submit a PR

it is quite likely that if you prefix file: to the paths it will work. try your luck

@ptrthomas
Copy link
Member

ptrthomas commented Mar 15, 2021

note to all - this appears to be a very un-conventional way of deriving paths:

karate.callSingle(karate.properties['karate.config.dir'] + '/features/auth/auth.feature');

quite likely there are alternate / better ways such as not using the karate.config.dir prop - which I think we internally update to an absolute path (which is the source of this problem probably), and karate.callSingle('file:features/auth/auth.feature') may work. note that karate supports a "working directory" flag now on the CLI as -w or --workdir which may be the solution here

edit: I would even try omitting the file: prefix and not start with a '/'

@JoshSchreuder
Copy link
Author

@ptrthomas yeah my apologies, I didn't have anything setup to build a new JAR off the source at the time.

Anyway, switching to file: seems to work quite well I think, I will try it out with my main project. If overriding karate.config.dir is not supported and for internal use only I'm fine if you want to close this if the workaround works just as well.

@ptrthomas
Copy link
Member

@JoshSchreuder awesome - thanks ! yes, I'll close this and not add a note to the release-notes as the likelihood of others running into this is really low

later - do consider suggesting any tips we can add for windows users using the stand-alone - that we can add to the docs as to how to better structure projects.

@JoshSchreuder
Copy link
Author

Got it working with a combination of custom argument, and --configdir.

My usecase is admittedly a bit unusual perhaps, I am running the JAR file from the VS Code extension via command.

A couple more things I found that I didn't spot in the breaking changes, not sure if bugs or intentional changes but I can put a repro up if you want:

  • And match response.data.size() == 50 no longer works for arrays in JSON response (failed due to: Unknown identifier: size)
  • Does headerno longer works inside ofbackground` blocks? I have to specify it at the scenario level for it to work.

@ptrthomas
Copy link
Member

use response.data.length I'll update the notes if needed

headers should propagate, so I'll need more details

@JoshSchreuder
Copy link
Author

use response.data.length I'll update the notes if needed

That was my first thought, but that didn't work either:

match failed: EQUALS
  $ | actual path does not exist (STRING:NUMBER)
  '#notpresent'
  50

headers should propagate, so I'll need more details

I'll put up a repro case for this.

@ptrthomas
Copy link
Member

cc @kirksl to be aware of VS Code extension usage

actually we prefer karate.sizeOf(array) since we don't want to trust the JS engine to do array length calcs

@JoshSchreuder
Copy link
Author

JoshSchreuder commented Mar 15, 2021

Oh I worked out the length thing, needed to change to And assert response.data.length == 50

@JoshSchreuder
Copy link
Author

Oh and finally... getting an exception when running in multi threaded mode with my callSingle in karate.config.js (I think that's why).

org.graalvm.polyglot.PolyglotException: Multi threaded access requested by thread Thread[pool-1-thread-1,5,main] but is not allowed for language(s) js.
- com.oracle.truffle.polyglot.PolyglotEngineException.illegalState(PolyglotEngineException.java:132)
- com.oracle.truffle.polyglot.PolyglotContextImpl.throwDeniedThreadAccess(PolyglotContextImpl.java:727)
- com.oracle.truffle.polyglot.PolyglotContextImpl.checkAllThreadAccesses(PolyglotContextImpl.java:627)
- com.oracle.truffle.polyglot.PolyglotContextImpl.enterThreadChanged(PolyglotContextImpl.java:526)
- com.oracle.truffle.polyglot.PolyglotEngineImpl.enter(PolyglotEngineImpl.java:1857)
- com.oracle.truffle.polyglot.HostToGuestRootNode.execute(HostToGuestRootNode.java:104)
- com.oracle.truffle.polyglot.PolyglotMap.entrySet(PolyglotMap.java:119)

I'll try and repro this minimally also.

@ptrthomas
Copy link
Member

ugh yes, that's an edge case with the match parser which I hadn't considered.

just for completeness, this should work (long story): * match (response.data.length) == 50 will see if this can be improved

and the exception is very bad news :| cc @joelpramos

if your callSingle is setting up functions, try removing those and stick to pure data. else well. we need to dig

@JoshSchreuder
Copy link
Author

JoshSchreuder commented Mar 15, 2021

if your callSingle is setting up functions, try removing those and stick to pure data. else well. we need to dig

My callSingle runs a feature that does a bunch of auth calls to grab user tokens for the pending test run. I guess it's probably reproducible with an API call inside callSingle but I will give it a go and see if I can minimally repro it.

ptrthomas added a commit that referenced this issue Mar 16, 2021
that tries to simulate edge cases that the graal engine dont like
ref: https://stackoverflow.com/q/66651979/143475 and #1515
@ptrthomas
Copy link
Member

@JoshSchreuder & @joelpramos - we have another report of the callSingle() issue here: https://stackoverflow.com/q/66651979/143475

I've asked the OP to comment in this thread. and meanwhile I tried my best to replicate the situation of a callSingle + feature + HTTP call in our "parallel" test that already mixes a callonce, here is the commit: 89f6816

and it still seems to work fine. but hopefully y'all will be able to see what's that extra bit in your tests that causing this problem.

@joelpramos
Copy link
Contributor

joelpramos commented Mar 16, 2021

@JoshSchreuder do you have a karate-config.js / karate-base.js with functions and are you using those inside the feature called by callSingle()?

@ptrthomas I might only be able to look at this on the weekend or later but was something that I don't think your additional tests included.

@ptrthomas ptrthomas changed the title FileNotFoundException in callSingle inside karate.config.js (1.0) JS / Multi threaded access error when using karate.callSingle() Mar 16, 2021
@ptrthomas ptrthomas added this to the 1.0.1 milestone Mar 16, 2021
@ptrthomas
Copy link
Member

reopening and making sure @aleruz is part of the conversation :)

@ptrthomas ptrthomas reopened this Mar 16, 2021
@JoshSchreuder
Copy link
Author

do you have a karate-config.js / karate-base.js with functions and are you using those inside the feature called by callSingle()?

My karate-config, minimally, is basically what's in https://github.com/JoshSchreuder/karate-repro in terms of callSingle. The results of the callSingle get stored and reused across test runs (basically a authenticated token cache).

karate-config.js callSingle -> auth.feature -> karate.config.js store variable

@ptrthomas
Copy link
Member

ptrthomas commented Mar 17, 2021

@JoshSchreuder I'm on a mac and I made this one change to karate-config.js to use file:: (EDIT in your git repro: https://github.com/JoshSchreuder/karate-repro )

function fn() {
  karate.callSingle('file:features/auth/auth.feature');
}

and ran this command. btw karate has some improvements, it will look for karate-config.js in the working folder by default and even logback.xml:

java -jar karate.jar features/auth/auth.feature -T 3

and everything ran fine for karate 1.0. which brings me to ask cc @aleruz by any chance are you both on windows ?

EDIT: I made sure to add a second scenario to ensure they run in parallel. note that you get a target/karate-reports/karate-timeline.html to look at

@aleruz
Copy link
Contributor

aleruz commented Mar 17, 2021

@ptrthomas I am on Mac as well. I am trying to debug right now, and found two things which seems to change the result of my test making it pass. But I need a bit more time to test it. At the moment, my suspicion is on two parallel runners which I created with different tags but which may be interfering in some tests (that was an experiment I did and that did not create issue with 0.9.6). Removing one of the two, seems to let the test pass now. But, I want to be sure. Will keep you posted here.

@aleruz
Copy link
Contributor

aleruz commented Mar 17, 2021

So, the callSingle works ok when a single feature is calling that. Tested it with a single test.
Then, duplicated the test and run 3 of them with "parallel" set to "5" --> ERROR.
Changed the "parallel" to 1 --> SUCCESS.

final Results results =
                    Runner.path("classpath:java").tags(tags)
                            .outputCucumberJson(true)
                            .reportDir("target/surefire-reports-" + tenant).parallel(1);

as said, with "1" it works. With more, it fails.

In the feature called with callSingle there is another callSingle and a normal call to other features.

@ptrthomas
Copy link
Member

FYI the single line of code with which I was able to reproduce: 43d8bf7#diff-6365020418ae2a40912aa4f001d49d2bbf988410171accd760333fea2b5b98acR3

image

@aleruz
Copy link
Contributor

aleruz commented Mar 27, 2021

Seems in my use case this is making no difference. :-(

@ptrthomas
Copy link
Member

@aleruz that doesn't help at all. I'm proposing to set a rule that karate.callSingle() can only return a JSON and NO functions. is everyone ok with this ? can you remove all functions in your karate.callSingle() and see what happens ?

@joelpramos
Copy link
Contributor

@aleruz question on your scenario, you said your feature called by callSingle() does a call twice to the same feature with different parameters.

Few questions on that - are those parameters reused within that call and how does that loop work? Any chance you can drop a screenshot here of that piece only and take out any private names / paths you might have?

ptrthomas added a commit that referenced this issue Mar 27, 2021
here added a second callSingle which uses results of the first callSingle
and the first callSingle includes a function in the results
ptrthomas added a commit that referenced this issue Mar 27, 2021
problem 1: very bad bug in initMagicVariables where we were setting __arg to a wrapper not the raw object
problem 2: we had completely missed hydrating / re-attaching magic variables
hopefully that explains the randomness, the incoming __arg may contain nested objects etc
@ptrthomas
Copy link
Member

okay one more time, my last commit is again a simple one but with one critical fix for "magic variables" that I really hope is the end of it

would be great if everyone here can try this version

@ptrthomas ptrthomas self-assigned this Mar 27, 2021
@joelpramos
Copy link
Contributor

joelpramos commented Mar 27, 2021

Tested the scenarios I had with that additional fix and works great. Thanks @ptrthomas , will check in a few things on top of that test for completeness that I tried in my local. @aleruz keep us posted on your results.

@aleruz
Copy link
Contributor

aleruz commented Mar 27, 2021

@ptrthomas my previous comment was referring to the fact I checked out and built your commit, but it did not help in my situation. I did it also with the latest commit, now.
@joelpramos Did the same with your commit, but also no luck.

https://github.com/aleruz/karate-call-single-bug/blob/main/src/test/java/examples/products/ProductDetails.feature
https://github.com/aleruz/karate-call-single-bug/blob/main/src/test/java/examples/products/commons/CreateProductTestDataForCurrentTenant.feature
https://github.com/aleruz/karate-call-single-bug/blob/main/src/test/java/examples/products/commons/CreateProductTestData.feature

I add here an extract of the tests, not runnable, but with what should be needed to understand the structure.

To recap, what I found to work have been:

@ptrthomas
Copy link
Member

sorry I can't make sense of what @aleruz is saying. is that a runnable example ? can someone translate all that above into a simple example which I can use to replicate the problem ? can anybody else run the latest and confirm the results ?

@aleruz
Copy link
Contributor

aleruz commented Mar 28, 2021

@ptrthomas : I did run my code in my environment with the commits you and @joelpramos did. This did not fix my specific case.

Then @joelpramos asked me for screenshots or code examples to understand the structure, and I committed my tests, which of course won't work because they miss all the configurations.

As I said, I found a workaround and can work with that (explained in my comment above). I tried to reproduce this structure in the examples in https://github.com/aleruz/karate-call-single-bug/tree/main/src/test/java/examples/users

mvn test "-Dkarate.options=--tags @user"

but still no luck. Will try further tomorrow.

@ptrthomas
Copy link
Member

@aleruz okay no worries ! sorry I'm just keen to get this solved. myself and @joelpramos are confident that the latest version solves all issues, so I really hope you haven't missed any step when building. maybe we can take this offline and get on a call next week if you are okay with that.

meanwhile I'm thinking of releasing over the next 2 days, there are a few important fixes, and we can hopefully reduce the cases where there are issues.

one thing would help. if you change all your callSingle() calls to call() does that make a difference ? do try confirm !

@aleruz
Copy link
Contributor

aleruz commented Mar 28, 2021

@ptrthomas I'll get another try tomorrow. For building, I did remove the 2.0.0 package in the local maven, then fetched your commit (exactly the commit ID), built as suggested in the "development" page, run the test again.
I am also ok with a call. I would suggest to align on Slack.

@ptrthomas
Copy link
Member

@aleruz one more thing - and this is important. are you calling any Java code that returns Java objects that contain data coming from karate and/or JS. in that case, ALL BETS ARE OFF :)

ptrthomas added a commit that referenced this issue Mar 28, 2021
…1515

and also make sure that cached result if an exception is detached from the js engine
@ptrthomas
Copy link
Member

one more try and a small code-change again: 4bac49c#diff-12a620743447978c823403798f5740cacc1eb990fb5603d015760002e5096431R175

I realized that line 175 (before this commit) could be entered by multiple threads (after a callSingle result was cached)
which would blow up because we use the JS engine to "attach"

anyone who can test this and report back would help !! I know I keep saying this - but this time I think it should work ...

ptrthomas added a commit that referenced this issue Mar 28, 2021
when we re-attach a callSingle result into the context
explanation: when we detach, we just patch any js functions and replace them with JsFunction
but we leave parent maps and lists as it
which means when we attach and update existing object references - the cache gets corrupted
the next time we try an attach we will encounter a stale graal value etc where we expected a JsFunction
solution is to deep-clone when we attach and this should be required only for a callSingle
@aleruz
Copy link
Contributor

aleruz commented Mar 28, 2021

@ptrthomas 🎉 seems this time is THE time 🎉 In my case no more issue! 👏🏻 Sad I could not reproduce it in an own separate example.

@ptrthomas
Copy link
Member

thank you @aleruz ! I think we are ready to release

ptrthomas added a commit that referenced this issue Mar 29, 2021
- make sure that when we detach, it is always a deep-clone
- use the same attach routine for a callonce
- make sure that the async scenario-listener that occsionally sees that js thread issue in ci does a detach-deep
- some defensive programming for the attach deep routine
ptrthomas added a commit that referenced this issue Mar 29, 2021
dont un-necessarily attach for callonce
now we dont need the extra synchronize for callsingle
ptrthomas added a commit that referenced this issue Mar 29, 2021
@ptrthomas
Copy link
Member

1.0.1 released

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

6 participants