Skip to content

src: manage handles more explicitly in cares_wrap, node_stat_watcher #21093

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
apapirovski wants to merge 5 commits into from
Closed

src: manage handles more explicitly in cares_wrap, node_stat_watcher #21093

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0bf2b28
src: do not persist timer handle in cares_wrap
apapirovski Jun 6, 2018
c62b30a
src: do not persist fs_poll handle in stat_watcher
apapirovski Jun 6, 2018
2897c15
test: check gc does not resurrect the loop
apapirovski Jun 6, 2018
95fc191
fixup: addaleax feedback
apapirovski Jun 6, 2018
1156ee2
fixup: bnoordhuis feedback
apapirovski Jun 7, 2018
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: do not persist timer handle in cares_wrap 
Instead of relying on garbage collection to close the timer handle,
manage its state more explicitly.
  • Loading branch information
@apapirovski
apapirovski committed Jun 7, 2018
commit 0bf2b28ef0bb1bb1cd057b67fc6f8d0229907bb4
46 changes: 23 additions & 23 deletions src/cares_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ class ChannelWrap : public AsyncWrap {

void Setup();
void EnsureServers();
void CleanupTimer();
void StartTimer();
void CloseTimer();

void ModifyActivityQueryCount(int count);

Expand Down Expand Up @@ -313,13 +314,7 @@ void ares_sockstate_cb(void* data,
if (read || write) {
if (!task) {
/* New socket */

/* If this is the first socket then start the timer. */
uv_timer_t* timer_handle = channel->timer_handle();
if (!uv_is_active(reinterpret_cast<uv_handle_t*>(timer_handle))) {
CHECK(channel->task_list()->empty());
uv_timer_start(timer_handle, ChannelWrap::AresTimeout, 1000, 1000);
}
channel->StartTimer();

task = ares_task_create(channel, sock);
if (task == nullptr) {
Expand Down Expand Up @@ -349,7 +344,7 @@ void ares_sockstate_cb(void* data,
channel->env()->CloseHandle(&task->poll_watcher, ares_poll_close_cb);

if (channel->task_list()->empty()) {
uv_timer_stop(channel->timer_handle());
channel->CloseTimer();
}
}
}
Expand Down Expand Up @@ -490,15 +485,26 @@ void ChannelWrap::Setup() {
}

library_inited_ = true;
}

/* Initialize the timeout timer. The timer won't be started until the */
/* first socket is opened. */
CleanupTimer();
timer_handle_ = new uv_timer_t();
timer_handle_->data = static_cast<void*>(this);
uv_timer_init(env()->event_loop(), timer_handle_);
void ChannelWrap::StartTimer() {
if (timer_handle_ == nullptr) {
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jun 7, 2018

Choose a reason for hiding this comment

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

if (timer_handle_ != nullptr) return; - saves a level of indent.

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Jun 7, 2018

Choose a reason for hiding this comment

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

It's a bit more complex if you look below. We also check if the handle is active in an else if. (We need to be able to call uv_timer_start to restart the timer if it expired but the handle still exists.)

timer_handle_ = new uv_timer_t();
timer_handle_->data = static_cast<void*>(this);
uv_timer_init(env()->event_loop(), timer_handle_);
} else if (uv_is_active(reinterpret_cast<uv_handle_t*>(timer_handle_))) {
return;
}
uv_timer_start(timer_handle_, AresTimeout, 1000, 1000);
}

void ChannelWrap::CloseTimer() {
if (timer_handle_ == nullptr)
return;

env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; });
Copy link
Copy Markdown
Member

@addaleax addaleax Jun 6, 2018

Choose a reason for hiding this comment

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

tbh, I don’t quite understand why this side-steps the beforeExit problematic? This can still be called from GC, right?

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Jun 6, 2018

Choose a reason for hiding this comment

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

In like 99.99% of cases, no. This is for the absolute edge cases where there's an error and we live through it. In most cases it'll get cleared via ares_sockstate_cb.

timer_handle_ = nullptr;
}

ChannelWrap::~ChannelWrap() {
if (library_inited_) {
Expand All @@ -508,17 +514,10 @@ ChannelWrap::~ChannelWrap() {
}

ares_destroy(channel_);
CleanupTimer();
CloseTimer();
}


void ChannelWrap::CleanupTimer() {
if (timer_handle_ == nullptr) return;

env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; });
timer_handle_ = nullptr;
}

void ChannelWrap::ModifyActivityQueryCount(int count) {
active_query_count_ += count;
if (active_query_count_ < 0) active_query_count_ = 0;
Expand Down Expand Up @@ -566,6 +565,7 @@ void ChannelWrap::EnsureServers() {
/* destroy channel and reset channel */
ares_destroy(channel_);

CloseTimer();
Setup();
}

Expand Down