-
Notifications
You must be signed in to change notification settings - Fork 355
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
ToObject: It is more thread safe to use isolate passed explicitly rather than relying on it retrieved via a static call in constructor. #310
Conversation
7535eea
to
e2a8cc5
Compare
Still, didn't manage to fix it but situation is better, only one crash in 1,5 days across 16 machines
|
This is another case where thread local isolate does not point to the same Isolate like J2V8 is pointing.
|
Again I think, i::Isolate::Current(); is wrong, it is accessed from thread locals: It somehow gets desynchronized with what is in V8 runtime defined in J2V8 in cpp layer. from isolate.h:
|
Another idea is that when Isolates desynchronize then check if Isolate pointer from here: is pointing to the same Isolate like V8 in V8 (v8RuntimePtr = _createIsolate(globalAlias);)
If it is not pointing this means V8 runtime has been AUTO released. |
@irbull Have you noticed that when Isolate::Scope is created we immediately call enter on it in constructor?
this means that when we createIsolate in Java_com_eclipsesource_v8_V8__1createIsolate, then we immediately enter isolate... |
@irbull I do see the crash and I exactly know why |
The reason is that, this constructor does not take isolate and we cannot pass it to it:
I have been trying to create a new constructor (overloaded that takes Isolate and uses it instead of this broken ThreadLocals that get desynchronized over time. Due to my rusty C++ knowledge I cannot do this, not to mention that this will require patching V8, which is not ideal... The actual issue is in this line:
so over time due to some race conditions or whatever(I don't know), I think this gets desynchronized with isolate defined in V8 |
So is that a V8 thing? Do you think there is a workaround we can use in J2V8? |
That is a very good question, it could be a bug in V8 itself or our misuse of it. Generally those ThreadLocals and i::Isolate::Current(); are evil but I really don't know how to fix this elegantly I was hoping you may know. |
I'll take a look. Still getting out of vacation mode :). Also, every time I venture back in to the .cpp files, I have to relearn the V8 API. |
IMHO, merging my PRs won't hurt but it doesn't fix the situation 100% |
I also looked in master of V8, latest, latest, there are still comments about removal and deprecation of:
|
They are basically storing Isolate in ThreadLocals. From isolate.h:
|
I agree. This is great work! I will get these PRs merged and then look through any other uses we have (I guess you didn't see any other places we explicitly use |
@irbull yes, two heads are better than one, I did some ground work this week to understand better the nature of the problem. What I still have no idea is why those ThreadLocals get desynchronized so that Isolate both pointers the one in C++ later V8 and those ThreadLocals in V8 code get desynced. There are a few schools of thought how to continue further IMHO:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thank you for merging PR. If you do not mind I will create a new issue about an unsolved crash and copy there investigation notes and our conversation as a log to document it.
|
That would be great! |
Done #313 |
Fixes #309. Perhaps there is more elegant fix but this already helps us in production on multi concurrent access and our rendering pool from V8s to serve pages.