MemoryPressureNotification got executed on the wrong platform

[levimm]

I'm using the new MultiIsolatePlatform feature added in v9.3.0.
I have following code running in a new node isolate which has its own thread loop. But it have a check failure error when trying to find isolate on the platform.

static node::MultiIsolatePlatform* platform = node::CreatePlatform(4, nullptr);
node::IsolateData* isolateData = node::CreateIsolateData(thread_isolate, thread_loop, platform);
thread_isolate->MemoryPressureNotification(v8::MemoryPressureLevel::kCritical);

The thread isolate is registered to the newly create platform when creating isolate data in line 2.
However, line 3 leads to a CHECK failure when trying to find this thread isolate because the platform instance here is returned from V8::GetCurrentPlatform() and it's not the one I created. Here's the callstack:

 1: node::Abort
 2: node::Assert
 3: node::NodePlatform::ForIsolate
 4: node::NodePlatform::CallDelayedOnForegroundThread
 5: v8::internal::Heap::CheckMemoryPressure
 6: v8::internal::Heap::MemoryPressureNotification
 7: v8::Isolate::MemoryPressureNotification

I'm not sure if I'm using this feature correctly. Could somebody advise?

[bnoordhuis]

The stack trace suggests you haven't actually called CreateIsolateData() yet. Check in a debugger what platform->per_isolate_ contains at the time of the crash.

[levimm]

@bnoordhuis platform->per_isolate_ contains the node main isolate.
As I mentioned, I run the previous code in a newly created separate isolate. And the thread isolate is added to the platform I created. But V8::GetCurrentPlatform gives me another platform, which I think is the main platform.

[bnoordhuis]

Right, that's not going to work. The Platform is a singleton, there can only be one instance per process.

[levimm]

@bnoordhuis Thank you for the explanation.
Is there any API to get the platform instance so I can register isolate into that?

[bnoordhuis]

static_cast<node::NodePlatform*>(v8::V8::GetCurrentPlatform()) should do it.

[levimm]

But this one is declared as V8_EXPORT_PRIVATE and it's not in the v8.h for external usage.

[bnoordhuis]

No, but you mentioned that you were already using it. There's no public API at the moment.

You shouldn't really need one either because as an embedder you control the platform instance. If you are trying to create a second node instance in another thread - don't, node is not thread-safe.

[levimm]

Actually I'm not directly using v8::V8::GetCurrentPlatform() since I can't.
I'm calling MemoryPressureNotification and then node internal code invokes V8::GetCurrentPlatform(). Is it possible to add something like node::GetCurrentPlatform to return this instance?

The Platform is a singleton, there can only be one instance per process.

Btw, what's the point of recently exported node::CreatePlatform to create another platform if this is singleton?

[bnoordhuis]

Is it possible to add something like node::GetCurrentPlatform to return this instance?

Can you go in more detail about what you are trying to do?

As I mentioned, if you are the embedder, then you already control the platform; and if you are not the embedder, then there should be no need to touch the platform.

Btw, what's the point of recently exported node::CreatePlatform to create another platform if this is singleton?

It does things V8's basic platform doesn't. (Not that I personally think it's useful to export, or at least for the reasons it was made public for, but that's a separate discussion.)

[levimm]

Thanks for the explanation.
I'm trying to build a multi-thread addin by creating new isolates in new thread (I'll handle the thread sync issue). I need a way to register those new isolates in the default platform with only node dependency.

[bnoordhuis]

You could open a PR that adds that API and see how it's received. Grep src/node.cc for v8_platform, it's its platform_ member.

I'll handle the thread sync issue

I'm curious how. IIUC, that's only safe when you suspend the main thread until your thread is done but that kind of heavy-weight serialization defeats the purpose of using threads.

[devsnek]

@bnoordhuis I'm actually doing mostly the same thing, atm if you use them correctly it's totally worth it. while serdes is definitely a pain if you aren't moving obscenely large objects around it's fine.

[bnoordhuis]

Are we talking about the same thing? I mean serialization as in synchronization, the opposite of parallelization, not data marshaling.

[levimm]

I'll try to follow the guideline and create a pr.
Actually my addon doesn't need to communicate between threads. Each thread communicates to another component through tcp socket.

[levimm]

I tried to register this new isolate in the default platform. MemoryPressureNotification works fine.
But I got another error when the js script is about to finish:

 	node.exe!v8::HandleScope::Initialize(v8::Isolate * isolate) Line 1023	C++
 	node.exe!v8::HandleScope::HandleScope(v8::Isolate * isolate) Line 1013	C++
 	node.exe!node::InternalCallbackScope::InternalCallbackScope(node::Environment * env, v8::Local<v8::Object> object, const node::async_context & asyncContext, node::InternalCallbackScope::ResourceExpectation expect) Line 1292	C++
>	node.exe!node::PerIsolatePlatformData::RunForegroundTask(std::unique_ptr<v8::Task,std::default_delete<v8::Task> > task) Line 168	C++
 	node.exe!node::PerIsolatePlatformData::RunForegroundTask(uv_timer_s * handle) Line 184	C++
 	node.exe!uv_process_timers(uv_loop_s * loop) Line 189	C
 	node.exe!uv_run(uv_loop_s * loop, uv_run_mode mode) Line 503	C

The cause is this line when trying to get Environment of this new isolate.

I tried two ways in order to fix this:

1. Invoke node::CreateEnvironment(isolate_data, isolate_context, 0, nullptr, 0, nullptr); but got more HandleScope error.
2. Try to node::FreeIsolateData(isolate_data); as soon as I can to avoid running foreground tasks. Didn't work either.

Could you advise?

[levimm]

I made it work by shutdown the original platform and then initialize it again with multiIsolatePlatform.
Does it sounds reasonable? @bnoordhuis

[bnoordhuis]

If it works, it works. The shutdown takes place while there is no active isolate? I think you should be good in that case.

[levimm]

Thanks