Skip to content

src: use env’s instead of isolate’s RequestInterrupt() + v8::Platform #32523

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.

Sign up for GitHub

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

Closed
addaleax wants to merge 7 commits into from
Closed

src: use env’s instead of isolate’s RequestInterrupt() + v8::Platform #32523

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
171409a
src: make `Environment::interrupt_data_` atomic
addaleax Mar 22, 2020
4de323c
src: fix cleanup hook removal for InspectorTimer
addaleax Mar 27, 2020
23b2da6
src: use env->RequestInterrupt() for inspector io thread start
addaleax Mar 22, 2020
62889df
src: use env->RequestInterrupt() for inspector MainThreadInterface
addaleax Mar 22, 2020
d9689db
fixup! src: fix cleanup hook removal for InspectorTimer
addaleax Mar 27, 2020
aeb354d
fixup! src: make `Environment::interrupt_data_` atomic
addaleax Mar 28, 2020
7e75cc1
src: flush V8 interrupts from Environment dtor
addaleax Mar 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
src: make Environment::interrupt_data_ atomic 
Otherwise this potentially leads to race conditions when used in a
multi-threaded environment (i.e. when used for its very purpose).
  • Loading branch information
@addaleax
addaleax committed Mar 27, 2020
commit 171409a2aad51ceb5d12e7b6799770fa12340e60
16 changes: 10 additions & 6 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,8 @@ Environment::Environment(IsolateData* isolate_data,
}

Environment::~Environment() {
if (interrupt_data_ != nullptr) *interrupt_data_ = nullptr;
if (Environment** interrupt_data = interrupt_data_.load())
*interrupt_data = nullptr;

// FreeEnvironment() should have set this.
CHECK(is_stopping());
Expand Down Expand Up @@ -735,12 +736,15 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) {
}

void Environment::RequestInterruptFromV8() {
if (interrupt_data_ != nullptr) return; // Already scheduled.

// The Isolate may outlive the Environment, so some logic to handle the
// situation in which the Environment is destroyed before the handler runs
// is required.
interrupt_data_ = new Environment*(this);
Environment** interrupt_data = new Environment*(this);
Environment** dummy = nullptr;
if (!interrupt_data_.compare_exchange_strong(dummy, interrupt_data)) {
delete interrupt_data;
return; // Already scheduled.
}
Comment on lines +774 to +779

@mmarchini mmarchini Mar 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's worth adding a comment here explaining what this code does.

@addaleax addaleax Mar 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmarchini Done, PTAL :)


isolate()->RequestInterrupt([](Isolate* isolate, void* data) {

@mmarchini mmarchini Mar 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember if this callback runs asynchronously. Any chance we could get a situation where we request the interrupt here but don't run it before the Isolate is deleted? If that's possible, we would have a leak on interrupt_data.

@addaleax addaleax Mar 27, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right – that is a possibility, but I don’t quite know what to do about it (without changing V8 APIs). The leak was already there before, too, and it’s limited to the size of a pointer…

@addaleax addaleax Mar 27, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I have an idea. Let me see. :)

@mmarchini mmarchini Mar 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. If ASAN starts to complain, we can force it to ignore this pointer (for now ASAN is happy, so we can leave it as is).

@addaleax addaleax Mar 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmarchini 7e75cc1 should address this by flushing interrupts when freeing the Environment instance … feels a bit like a hack to me but I think it’s the best we can do :)

std::unique_ptr<Environment*> env_ptr { static_cast<Environment**>(data) };
Expand All @@ -751,9 +755,9 @@ void Environment::RequestInterruptFromV8() {
// handled during cleanup.
return;
}
env->interrupt_data_ = nullptr;
env->interrupt_data_.store(nullptr);
env->RunAndClearInterrupts();
}, interrupt_data_);
}, interrupt_data);
}

void Environment::ScheduleTimer(int64_t duration_ms) {
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1442,7 +1442,7 @@ class Environment : public MemoryRetainer {

void RunAndClearNativeImmediates(bool only_refed = false);
void RunAndClearInterrupts();
Environment** interrupt_data_ = nullptr;
std::atomic<Environment**> interrupt_data_ {nullptr};
void RequestInterruptFromV8();
static void CheckImmediate(uv_check_t* handle);

Expand Down