Skip to content

fix: make process.uptime() return the correct time #19436

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

Jump to bottom
Merged
jkleinsc merged 1 commit into from
Jul 30, 2019
Merged

fix: make process.uptime() return the correct time #19436

jkleinsc merged 1 commit into from
Jul 30, 2019

Conversation

@deermichel
Copy link
Contributor

@deermichel deermichel commented Jul 24, 2019 •
edited
Loading

Description of Change

Fixes #19425.

The issue was rooted in an upstream change: nodejs/node#26016 (found its way to electron in #17752). Previously, the process-relative uptime was set in a method which got called by both node::Start (used by nodejs) and node::Init (used by embedders, deprecated) init methods. After the change, it is set directly in node::Start and thereby not when using node::Init. This PR just reverts that.

Since i don't see any difference in either way of setting the variable, we might wanna upstream this patch so other embedders can benefit from it as well.

cc @MarshallOfSound @codebytere

Checklist

Release Notes

Notes: Fixed process.uptime() returning the wrong time.

@deermichel deermichel requested a review from a team as a code owner July 24, 2019 19:07
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jul 24, 2019
@nornagon
Copy link
Contributor

nornagon commented Jul 24, 2019

@deermichel before we merge this, do you want to try submitting a PR to nodejs/node to see if anyone from the node maintainer group has thoughts about it?

@deermichel
Copy link
Contributor Author

deermichel commented Jul 24, 2019 •
edited
Loading

Yep, sounds good to me :) --> nodejs/node#28849

@deermichel deermichel mentioned this pull request Jul 24, 2019
Closed
2 tasks
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Jul 25, 2019
@deermichel
Copy link
Contributor Author

deermichel commented Jul 29, 2019

@nornagon my node PR got merged 🎉. So can we keep the patch here until we reached the current upstream version?

nornagon
nornagon approved these changes Jul 29, 2019
View reviewed changes
Copy link
Contributor

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

yup sgtm

@codebytere
Copy link
Member

codebytere commented Jul 29, 2019

@deermichel needs rebase ☝️

@deermichel deermichel force-pushed the intern/process-uptime branch from 74cbc05 to 7013160 Compare July 29, 2019 17:56
@deermichel
Copy link
Contributor Author

deermichel commented Jul 29, 2019

done ✅

MarshallOfSound
MarshallOfSound requested changes Jul 29, 2019
View reviewed changes
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Also now that this is fixed we should re-enable the test/parallel/test-process-uptime node test

@deermichel deermichel force-pushed the intern/process-uptime branch 2 times, most recently from ac60f36 to ebfefbf Compare July 29, 2019 22:10
@deermichel deermichel force-pushed the intern/process-uptime branch from ebfefbf to 09691d1 Compare July 29, 2019 22:11
@jkleinsc jkleinsc merged commit 6e367da into master Jul 30, 2019
@release-clerk
Copy link

release-clerk bot commented Jul 30, 2019

Release Notes Persisted

Fixed process.uptime() returning the wrong time.

@jkleinsc jkleinsc deleted the intern/process-uptime branch July 30, 2019 18:08
@trop
Copy link
Contributor

trop bot commented Jul 30, 2019

I was unable to backport this PR to "5-0-x" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jul 30, 2019

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

@trop trop bot removed target/5-0-x labels Jul 30, 2019
@codebytere
Copy link
Member

codebytere commented Aug 1, 2019

@deermichel could you please open manual backports for this to 5/6?

@deermichel
Copy link
Contributor Author

deermichel commented Aug 1, 2019

oh missed this, sure...

deermichel pushed a commit that referenced this pull request Aug 1, 2019
fix: make process.uptime() return the correct time (#19436)
7d5de0c
@deermichel deermichel mentioned this pull request Aug 1, 2019
Merged
@trop
Copy link
Contributor

trop bot commented Aug 1, 2019

A maintainer has manually backported this PR to "5-0-x", please check out #19566

@deermichel deermichel mentioned this pull request Aug 1, 2019
Merged
@trop
Copy link
Contributor

trop bot commented Aug 1, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #19567

deermichel pushed a commit that referenced this pull request Aug 1, 2019
fix: make process.uptime() return the correct time (#19436)
271719e
MarshallOfSound pushed a commit that referenced this pull request Aug 1, 2019
@MarshallOfSound
fix: make process.uptime() return the correct time (#19436) (#19566)
b4f5c4b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@codebytere codebytere codebytere approved these changes

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

+1 more reviewer

@nornagon nornagon nornagon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

process.uptime() returns unexpected result

6 participants

@deermichel @nornagon @codebytere @jkleinsc @MarshallOfSound