Skip to content
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

Fix timers #336

Closed
wants to merge 3 commits into from
Closed

Fix timers #336

wants to merge 3 commits into from

Conversation

loyd
Copy link
Contributor

@loyd loyd commented Jan 13, 2015

@@ -15,8 +15,6 @@
var common = require('../common');

var Timer = process.binding('timer_wrap').Timer;
Timer.now = function() { return ++Timer.now.ticks; };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I don't fully understand what @bnoordhuis meant. However, the test runs without it, i.e. check it out correctly this._onTimeout.

@trevnorris trevnorris added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jan 22, 2015
@brendanashworth
Copy link
Contributor

Sorry for letting this pull request fall through the cracks. I think it would be best (in the hope of merging) to split the pull request into separate PRs for each individual commit (and perform a rebase on each, as necessary). Some of these commits will probably bring more discussion than the others and I don't think these can all be merged at once.

@Fishrock123
Copy link
Contributor

@loyd are you still up to updating this PR? Going to close otherwise.

@brendanashworth
Copy link
Contributor

@Fishrock123 at least two of the three commits here I would like to see merged into the repository, but I think since they are packaged all together they aren't very welcomed. If you could hold off on closing this so I could resubmit the commits one at a time, that'd be great - I'd be up to update this for him.

@loyd
Copy link
Contributor Author

loyd commented Mar 4, 2015

Sorry for the delay in response.
@brendanashworth What kind of commits do you mean?

@brendanashworth
Copy link
Contributor

@loyd thanks for rebasing. I'd like to see ad60db8 merged as I believe that would be an easy semver-patch - and therefor should be treated in a separate pull request. 1c9e35a I would like to see merged in eventually, but it could very well be interpreted as a semver-major change and would take longer to merge in.

I'm not entirely sure what fe805ef fixes, so I'm not going to take stance on that - but as these commits individually are very different from eachother, putting them in one pull request is the fast track to not being merged and the only way I can see these getting merged would be re-submitting pull requests individually, one for each commit.

@brendanashworth
Copy link
Contributor

All the commits in this pull request need some serious rebasing by now, and since they need to be resubmitted in separate pull requests anyways, I'll close this for now. We'd still welcome new pull requests with these changes, but as they are now (bundled and in need of a rebase), we cannot continue to work on them from this PR. If you disagree, you can always reopen this. Thanks!

p.s. I tried rebasing the "fix for unref'd timers", but I got a segfault. 😛

@Fishrock123
Copy link
Contributor

I need to look at this tomorrow. See also #1151, #1152, and others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants