FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

WIP src: don't check closing handles on beforeExit by trevnorris · Pull Request #1317 · nodejs/node · GitHub

Repository navigation

WIP src: don't check closing handles on beforeExit - #1317

Closed
trevnorris wants to merge 1 commit into
nodejs:v1.xfrom
trevnorris:loop-alive-closing-handles
Closed

trevnorris wants to merge 1 commit into
nodejs:v1.xfrom
trevnorris:loop-alive-closing-handles

Conversation

Copy link
Copy Markdown
Contributor

uv_loop_alive() returns true if there are closing handles. This causes
an issue running the beforeExit event and a timer has been unref'd:

process.on('beforeExit', function() {
  setTimeout(function() { }, 100).unref();
});

The event loop also shouldn't run one additional time or else the timer
may execute an additional time.

This is a WIP commit, and mainly opened to raise discussion around the problem at hand.

R=@bnoordhuis

mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 1, 2015

thlorenz commented Apr 1, 2015

Copy link
Copy Markdown
Contributor

Fixes

// before-exit-hangs.js
'use strict';

process.on('beforeExit', function() {
  setInterval(function() {}, 1).unref();
});

This one would hang indefinitey

// unrefd-timers-execute-onexit.js
var assert = require("assert")
var intervals = 0
var i = setInterval(function () {
  intervals++
  i.unref()
  eatTime()
}, 10)

function eatTime() {
  // the goal of this function is to take longer than the interval
  var count = 0
  while (count++ < 1e7) {
    Math.random()
  }
}

process.on("exit", function () {
  assert.equal(intervals, 1)
})

Still failing

// test-regress-GH-io-1151.js
var assert = require('assert');

setImmediate(function () {
  require("http");
});

var i = setInterval(function () {
  setImmediate(process.exit)
}, 10);
i.unref();

process.on("exit", onexit);

function onexit() {
  assert.strictEqual(process._getActiveHandles().length, 0, 'should have no more active handles on exit')
}
// test-timers-unref-interval-exit-after-next.js
var assert = require('assert');

var i = setInterval(function () {
  // does not matter if we swap the nextTick with the setImmediate
  process.nextTick(function() {
    setImmediate(process.exit);
  })
}, 1);
i.unref();

process.on('exit', function() {
  assert.strictEqual(process._getActiveHandles().length, 0);
});

trevnorris force-pushed the loop-alive-closing-handles branch from 8b9cc25 to 583b570 Compare April 1, 2015 19:57

thlorenz commented Apr 1, 2015

Copy link
Copy Markdown
Contributor

I investigated into libuv a bit and found the following oddity:

If you call unref it won't remove handles if they are currently closing due to:
if (((h)->flags & UV__HANDLE_CLOSING) != 0) break;

Then when you call uv_loop_alive it'll return true if handles are closing due to loop->closing_handles != NULL;

So if we go around the loop while one handle is closing we won't exit, but during that loop a new handle could be created.

Not sure if I'm misunderstanding something here or if this could be at least one reason for this issue.

@saghul please correct me if I'm wrong here.

Copy link
Copy Markdown
Member

@thlorenz Your analysis is correct but I wouldn't call it an oddity, it's by design.

Close callbacks must run because that's the only place in the life cycle of a handle where it's safe for the libuv embedder to clean up resources associated with the handle. That's why closing an unref'd handle gives it an implicit ref.

Comment thread src/node.cc

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You're probably well aware of this but active_handles and active_reqs are implementation details.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah. That was done just to bring up the issue I'm working around, and the fact I can't figure out how to get around it.

trevnorris force-pushed the loop-alive-closing-handles branch from 583b570 to 4339d83 Compare April 1, 2015 20:27

Copy link
Copy Markdown
Contributor

Related to #1151

Fishrock123 added the timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout(). label Apr 1, 2015

Copy link
Copy Markdown
Contributor

Still failing

Both of the failing checks check for the leak #1152 was supposed to fix, and not the issue at hand.

@thlorenz Maybe they will work properly if you check the iterations?

uv_loop_alive() returns true if there are closing handles. This causes
an issue running the beforeExit event and a timer has been unref'd:

    process.on('beforeExit', function() {
      setTimeout(function() { }, 100).unref();
    });

The event loop also shouldn't run one additional time or else the timer
may execute an additional time.
trevnorris force-pushed the loop-alive-closing-handles branch from 4339d83 to 190af5e Compare April 1, 2015 20:50

thlorenz commented Apr 1, 2015

Copy link
Copy Markdown
Contributor

FWIW with this patch applied you can see from the logs that there is a race condition regarding active/closing handles when running unrefd-timers-execute-onexit.js test.

A timer gets started while the previous one finishes closing.

Excerpt

hasAH: 1, hasAR: 0, hasCH: 0
hasAH: 1, hasAR: 0, hasCH: 0
[ uv_unref ]
[--] -> [--] timer 0x105a00050::8

[ uv_timer_start ]
[--] -> [-A] timer 0x105a00050::10

hasAH: 0, hasAR: 0, hasCH: 1
hasAH: 0, hasAR: 0, hasCH: 1
[ uv_unref ]
[--] -> [--] timer 0x105a00050::10

[ uv_timer_start ]
[R-] -> [RA] timer 0x101f01bf0::11

hasAH: 1, hasAR: 0, hasCH: 0
hasAH: 1, hasAR: 0, hasCH: 0
[--] -> [--] timer 0x105a00050::12
  • hasAH: has active handles
  • hasAR: has active requests
  • hasCH: has closing handles

thlorenz commented Apr 1, 2015

Copy link
Copy Markdown
Contributor

@bnoordhuis I understand now why close callbacks must run (also talked to @trevnorris about this).
However if I can create a timer inside that close callback, I could get myself into an endless loop even if I unref it.

Is that correct? If yes do we need a solution or should we just tell people to not do that?

Copy link
Copy Markdown
Member

That's correct and I'm inclined to say it's more of a documentation issue than a technical issue.

Copy link
Copy Markdown
Contributor Author

After thinking on this, and the implications pointed out by @bnoordhuis with the need to run the uv_close() callback so resources can be properly cleaned up, I agree this is a documentation issue. Closing.

trevnorris closed this Apr 2, 2015
trevnorris deleted the loop-alive-closing-handles branch March 28, 2016 22:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL