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

Deferred function arguments change over time #704

Closed
Ghostbird opened this issue Jan 27, 2020 · 8 comments
Closed

Deferred function arguments change over time #704

Ghostbird opened this issue Jan 27, 2020 · 8 comments

Comments

@Ghostbird
Copy link

Ghostbird commented Jan 27, 2020

I'm stuck with a rather strange situation.
I have to asynchronously retry some CLR functions that are called from Jint, while ensuring their calling order is maintained.
Therefore, I defer such Func<JsValue, JsValue[], JsValue>s to a queue that tries the deferred function until it succeeds, and then continues with the next one.
The problem happens when I do something like this:

// JavaScript
MyCLRFunctionRefWithRetry('foo');
MyCLRFunctionRefWithRetry('bar');

Both functions are called, deferred and after 30 seconds the first is retried. At this point args[0] of the deferred function call MyCLRFunctionRefWithRetry('foo'); resolves to 'bar' instead of 'foo'.
Note that a lot of other stuff is going on around these function calls too, and they are not really called immediately after each other in the Jint engine.

Annoyingly I cannot reproduce this behaviour in my minimal-code example: https://github.com/Ghostbird/JintAsyncRetry

Could someone shed some light on why this happens? Maybe even suggest an avenue to fix it?
I've tried to integrate argument serialisation before deferring and de-serialisation on execution, however too much runtime type information is lost and the app goes down in a shower of InvalidCastExceptions.

@lahma
Copy link
Collaborator

lahma commented Jan 27, 2020

Can it happen that engine is called concurrently? Engine is not thread-safe.

@Ghostbird
Copy link
Author

I'm pretty sure that's not the case. The queue actually enforces that all methods are called consequently, as the minimal-code example shows.

@lahma
Copy link
Collaborator

lahma commented Jan 27, 2020

Ok, it's just that sometimes minimal example might lack behavior of the system as a whole. What version are you using?

@Ghostbird
Copy link
Author

Ghostbird commented Jan 28, 2020

The minimal code example was designed to cover the expected problematic factors such as multithreading. However it does not show the error.

On the other hand, I found that when I downgrade to the latests stable, I cannot reproduce the issue. The troublesome part is that I have always run the Jint 3 NuGet release, and I don't know whether my software is backwards compatible. Running a two year old version just to take advantage of an implementation detail seems like a bad idea to me. Additionally I have no proof that the error does not occur in the stable branch, and is just less likely to occur.

The whole behaviour reminds me of ARM ROP-ing where you enter a body function with the function arguments of an earlier called function. This is possible in ARM since the function arguments are passed in a fixed set of registers. However it doesn't make much sense in a high-level automatically managed memory environment such as C#. It would imply explicit (faulty) manual memory management built on top of a system that already provides automatic memory management.

It might however be the result of an overzealous optimisation technique introduced in Jint 3. If so, can anyone confirm the existence of such a technique and provide a way to disable it?

@lahma
Copy link
Collaborator

lahma commented Jan 28, 2020

Do you have javascript code that accesses function's implicit arguments object? Recent commit to dev changed the handling to be more robust. There's another fix in dev for string handling for concatenated strings. Both are available on dev feed, not yet in NuGet.org feed.

By running a branch do you mean you compile yourself or run NuGet.org packages?

@Ghostbird
Copy link
Author

Ghostbird commented Jan 28, 2020

I don't have code that accesses the implicit arguments, and the problem is not related to concatenated strings.
I edited my post above to clarify that I meant the Jint 3 NuGet release.

For now I'm solving it on a higher level in my code by spawning new Engines and copying the parts of the context we need to persist. So far this passes my tests, and seems like an acceptable solution.

I think time-complexity of Engine creation changes from O(ni) to O(ni+1) for my process. However I'm unsure of the exact value of i, and I expect this to still be a small part of my total process time-complexity, and therefore unlikely to heavily impact overall performance.

Should I close this issue – as my specific case has been worked around – or leave it open – as the general case remains unsolved?

@lahma
Copy link
Collaborator

lahma commented Jan 28, 2020

Good to hear that that fresh engine instances solved your problem. So far my suspicion is that this is related to multi-threading and not engine itself. There's multitude of more-or-less complex tests being run against the code and none exhibit such problematic behavior; recursion, nested calls etc all work as expected.

The v3 engine should be quite lightweight to instantiate and we are always happy to try to improve speed if you find bottlenecks.

If you are fine with closing maybe that's easier for now as no way to reproduce. We can always reopen if the problem surfaces again.

@Ghostbird
Copy link
Author

I'm pretty sure that the underlying cause is the stack contamination mentioned in #673. In my case it is not concurrent. But the stack of a deferred call may have been contaminated by a later call, by the time the first call is actually executed.

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

2 participants