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

Force uv loop cleanup #11465

Merged
merged 9 commits into from Jan 4, 2018

Conversation

Projects
None yet
4 participants
@ckerr
Member

ckerr commented Dec 18, 2017

This PR tries to ensure that a Worker's event loop has no pending business before the loop is closed. It does this by

  1. calling uv_stop() on the loop
  2. walking the event loop to close() all active handles
  3. calling uv_loop_run(UV_RUN_DEFAULT) so that pending handles can be disposed of properly

This is related to #11243

In addition, since uv_loop_new() and uv_loop_destroy() are deprecated API, this patch replaces those with uv_loop_init() and uv_loop_close()

@ckerr ckerr requested a review from electron/reviewers as a code owner Dec 18, 2017

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Dec 18, 2017

Is all this complex cleanup really needed?
The following changes do the trick of removing the assertion too.
(error handling is omitted for simplicity)

diff --git a/atom/common/node_bindings.cc b/atom/common/node_bindings.cc
index 03e96ae8a..618ca45c8 100644
--- a/atom/common/node_bindings.cc
+++ b/atom/common/node_bindings.cc
@@ -100,10 +100,15 @@ base::FilePath GetResourcesPath(bool is_browser) {
 
 NodeBindings::NodeBindings(BrowserEnvironment browser_env)
     : browser_env_(browser_env),
-      uv_loop_(browser_env == WORKER ? uv_loop_new() : uv_default_loop()),
       embed_closed_(false),
       uv_env_(nullptr),
       weak_factory_(this) {
+  if (browser_env == WORKER) {
+    uv_loop_ = new uv_loop_t;
+    uv_loop_init(uv_loop_);
+  } else {
+    uv_loop_ = uv_default_loop();
+  }
 }
 
 NodeBindings::~NodeBindings() {
@@ -119,9 +124,11 @@ NodeBindings::~NodeBindings() {
   uv_sem_destroy(&embed_sem_);
   uv_close(reinterpret_cast<uv_handle_t*>(&dummy_uv_handle_), nullptr);
 
-  // Destroy loop.
-  if (uv_loop_ != uv_default_loop())
-    uv_loop_delete(uv_loop_);
+  // Destroy the loop if needed.
+  if (browser_env_ == WORKER) {
+    uv_loop_close(uv_loop_);
+    free(uv_loop_);
+  }
 }
 
 void NodeBindings::Initialize() {
@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Dec 18, 2017

Although uv_loop_close(uv_loop_) returns UV_EBUSY, so yeah, the cleanup is probably required.

@ckerr

This comment has been minimized.

Member

ckerr commented Dec 18, 2017

Replacing uv_loop_destroy() with uv_loop_close() gets rid of the assertion because uv_loop_destroy() tests to confirm that uv_loop_close() succeeded. It removes the code that tests for the error, rather than fixing the error.

@ckerr

This comment has been minimized.

Member

ckerr commented Dec 18, 2017

...but it's a fair question! I removed the deprecated APIs before writing up the cleanup code andthought the same thing at first when the assertion went away 😄

@ckerr

This comment has been minimized.

Member

ckerr commented Dec 20, 2017

This seems to work and theappveyor failure seems to be a CI issue rather than related to this PR. Removing the [WIP] tag & ready for review

@ckerr ckerr changed the title from [WIP] Force uv loop cleanup to Force uv loop cleanup Dec 20, 2017

@@ -85,6 +85,9 @@ class NodeBindings {
// Whether the libuv loop has ended.
bool embed_closed_;
// Loop used when constructed in WORKER mode
uv_loop_t worker_loop_;

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 20, 2017

Contributor

It looks like this thing is only used to check if the loop is a worker loop.
But we can make the same check using browser_env_ == WORKER.
So would the code become simpler if worker_loop_ is removed?

This comment has been minimized.

@ckerr

ckerr Dec 20, 2017

Member

I think you might be looking at an older diff.

worker_loop_ is the loop when the constructor gets passed a Worker mode

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 20, 2017

Contributor

worker_loop_ is the loop when the constructor gets passed a Worker mode

Yes, but uv_loop_ = &worker_loop_;, so...
I mean using two things instead of one seems too complex to me. I might be wrong though )

This comment has been minimized.

@ckerr

ckerr Dec 20, 2017

Member

The upstream reason for deprecating uv_loop_new() was to remove unnecessary heap allocs, so I followed their lead by aggregating a uv_loop_t.

But a single variable is clearer though and I'm fine with either approach. If you want, I'll remove worker_loop_ and new/delete a uv_loop_t iff WORKER mode.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 21, 2017

Contributor

If you want

It's not a good reason ) Approved.

if (!uv_run(uv_loop_, UV_RUN_DEFAULT))
break;
DCHECK_EQ(uv_loop_alive(uv_loop_), 0);

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 20, 2017

Contributor

DCHECK(!uv_loop_alive(...)) maybe?
Looks like if (isAlive == false) now =)

This comment has been minimized.

@ckerr

ckerr Dec 20, 2017

Member

Sure. Committed in 26ace8d + df92c2d

if (uv_loop_ != uv_default_loop())
uv_loop_delete(uv_loop_);
// Clean up worker loop
if (uv_loop_ == &worker_loop_) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 20, 2017

Contributor

Will it be clearer if all the code under the if statement is moved to a separate method?

By using the // Clean up worker loop comment you essentially give a name to that block, and code block with a name sounds like a method to me.

This comment has been minimized.

@ckerr

ckerr Dec 20, 2017

Member

Good idea, committed 132d970

The steps would work on any loop being closed, not just this Worker, so I didn't put 'worker' in the name.

@zcbenz

zcbenz approved these changes Dec 21, 2017

The code looks good to me, I'm open to how worker_loop_ is dealt with.

@codebytere

This comment has been minimized.

Member

codebytere commented Jan 4, 2018

is this mergeable?

@codebytere codebytere merged commit 435c9c1 into master Jan 4, 2018

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@codebytere codebytere deleted the force-uv-loop-cleanup branch Jan 4, 2018

kwonoj added a commit to kwonoj/electron that referenced this pull request Jan 25, 2018

Force uv loop cleanup (electron#11465)
* ensure all uv handles are closed before ending worker's loop

* add DCHECK to test that the Worker loop is finished

* don't call deprecated uv_loop_new(), uv_loop_delete()

* make cpplint happy

* fix comment error

* empty commit for CI

* tweak DCHECK expression

* extract-method: stop_and_close_uv_loop()

* fix DCHECK oops
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment