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

[RFC] New API: powerMonitor "shutdown" event #11417

Merged
merged 11 commits into from Feb 5, 2018

Conversation

Projects
None yet
4 participants
@tarruda
Copy link
Contributor

tarruda commented Dec 12, 2017

In this PR I add a new event to powerMonitor. Usage is very simple:

powerMonitor.on('shutdown', (e) => {
  e.preventDefault()  // This will delay OS shutdown as much as possible
  someAppSpecificCleanup().then(() => {
    app.quit() // exit, this must be called ASAP!
  })
})

This is useful for any application that needs to ensure its state is saved correctly when the computer shuts down.

Currently only works on Linux, though I imagine windows/osx have similar APIs that could be used (I know I have seen Windows notification that some apps are delaying shutdown)

@tarruda tarruda requested a review from electron/reviewers as a code owner Dec 12, 2017

@tarruda tarruda force-pushed the power-monitor-shutdown-event-and-delay-api branch from da93e36 to e179fee Dec 12, 2017

LOG(ERROR) << "Invalid signal: " << signal->ToString();
return;
}
if (status) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 12, 2017

Contributor

Sorry for nitpicking, but status is a really bad name for a boolean variable.
It's hard to guess that true or false values might mean.

This comment has been minimized.

@tarruda

tarruda Dec 13, 2017

Author Contributor

How about using active as shown in the docs? I'm not sure if that name is better though.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 13, 2017

Contributor

active is better since it implies a binary state of active/inactive, while status can have numerous values, e.g. http status codes.

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

Maybe call it shuttingDown for simplicity's sake.

There's more nuance to it than that, but shuttingDown is good for how we're using it

This comment has been minimized.

@tarruda

tarruda Dec 14, 2017

Author Contributor

👍

@@ -5,4 +5,16 @@ const {powerMonitor, PowerMonitor} = process.atomBinding('power_monitor')
Object.setPrototypeOf(PowerMonitor.prototype, EventEmitter.prototype)
EventEmitter.call(powerMonitor)

powerMonitor.on('newListener', (event) => {
if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 12, 2017

Contributor

Is the new "shutdown" listener added only after this function is called?

This comment has been minimized.

@tarruda

tarruda Dec 13, 2017

Author Contributor

Yes

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

This seems like unnecessary hoops to jump through. I'd vote for removing this js bootstrap code and just auto-inhibiting in the C++ code, same as we do now for sleep.

before(async () => {
const calls = await getCalls()
assert.equal(calls.length, 2)
powerMonitor.once('shutdown', () => { })

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 12, 2017

Contributor

Why all of it should be done in a before hook?
I don't see how it prepares an environment or context for tests.

This comment has been minimized.

@tarruda

tarruda Dec 13, 2017

Author Contributor

Normally I try to structure BDD tests like this:

  • describe describes the context
  • beforeEach sets up the context
  • it describes/verifies an expectation about the context

In this case the context is "when a listener is added to shutdown event" which is configured in the before callback. The it call describes/verifies the "should call Inhibit to delay shutdown" expectation. I also added an assertion in the before callback to make it explicit that getCalls returns two calls prior to adding the listener.

This comment has been minimized.

@alexeykuzmin

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

What are the two calls that happen prior to adding the listener?

})

powerMonitor.on('removeListener', (event) => {
if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Dec 12, 2017

Contributor

Is a "shutdown" listener removed before this function is called?

This comment has been minimized.

@tarruda

tarruda Dec 13, 2017

Author Contributor

Yes

@ckerr
Copy link
Member

ckerr left a comment

This is a good feature to have and I'm happy that you're building on the DBus features you've already added. Thank you!

Please keep the last paragraph in mind as you read my review :) I think our previous design choices are coming back to bite us, and IMO we're better off fixing them now.

class PowerObserver : public base::PowerObserver {
public:
PowerObserver() {}
void BlockShutdown() {}

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

This method seems unnecessary, since we want to listen for shutdown as soon as there's any listener. Why not unconditionally register inPowerObserverLinux::OnLoginServiceAvailable, as we do with PrepareForSleep and PrepareForShutdown?

public:
PowerObserver() {}
void BlockShutdown() {}
void UnblockShutdown() {}

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member
  1. PrepareForShutdown and PrepareForSleep have the same arguments and concepts, but we're handling one by releasing the lock immediately after emit and the other has a new custom handler. Is this to avoid breakage in how sleep is handled on macOS and Windows? (And if it's not causing problems for sleep, do we need this extra custom handler for shutdown?)

  2. This method should be protected to emphasize that it's not public API, but rather is only intended for use by PowerMonitor.

@@ -16,6 +16,10 @@ const char kLogindServiceName[] = "org.freedesktop.login1";
const char kLogindObjectPath[] = "/org/freedesktop/login1";
const char kLogindManagerInterface[] = "org.freedesktop.login1.Manager";

// Store shutdown lock as a global, since we only want to release it when the
// main process exits.
base::ScopedFD shutdown_lock;

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

IMO this strengthens the argument for refactoring the DBus code out of Observer (see comment below about the Monitor<->Observer interdependencies):

  1. We shouldn't have a singleton impl for a nonstatic method

  2. We shouldn't have a ScopedFD that never goes out of scope

This comment has been minimized.

@tarruda

tarruda Dec 14, 2017

Author Contributor

I could move shutdown_lock into the class, but it is possible that the ScopedFD goes out of scope before the application has fully exited, which results in a small probability of systemd killing the app.

Maybe it is not a problem in practice since this class instance is probably deleted only a bit sooner than the app exits, but we would loose guarantee that the app exits cleanly by itself.

logind_->ConnectToSignal(kLogindManagerInterface, "PrepareForSleep",
base::Bind(&PowerObserverLinux::OnPrepareForSleep,
weak_ptr_factory_.GetWeakPtr()),
base::Bind(&PowerObserverLinux::OnSignalConnected,
weak_ptr_factory_.GetWeakPtr()));
// Take sleep inhibit lock

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

If the method name is so confusing that it needs a comment, maybe rename it InhibitSleep()?

This comment has been minimized.

@tarruda

tarruda Dec 14, 2017

Author Contributor

👍

This comment has been minimized.

@tarruda

tarruda Dec 14, 2017

Author Contributor

As I said in another comment, I planned to refactor the sleep Inhibit calls to use the same mechanism as shutdown Inhibit. So how about changing it to BlockSleep/UnblockSleep instead?

LOG(ERROR) << "Invalid signal: " << signal->ToString();
return;
}
if (status) {

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

Maybe call it shuttingDown for simplicity's sake.

There's more nuance to it than that, but shuttingDown is good for how we're using it

@@ -5,4 +5,16 @@ const {powerMonitor, PowerMonitor} = process.atomBinding('power_monitor')
Object.setPrototypeOf(PowerMonitor.prototype, EventEmitter.prototype)
EventEmitter.call(powerMonitor)

powerMonitor.on('newListener', (event) => {
if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

This seems like unnecessary hoops to jump through. I'd vote for removing this js bootstrap code and just auto-inhibiting in the C++ code, same as we do now for sleep.

before(async () => {
const calls = await getCalls()
assert.equal(calls.length, 2)
powerMonitor.once('shutdown', () => { })

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

What are the two calls that happen prior to adding the listener?

'b', [['b', true]])
})
})
})

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

If we keep the preventDefault approach, please add a test here to exercise it

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

And, I know I've been a big pain already, but please document the API in docs/api/power-monitor.md. Thank you! 😄

This comment has been minimized.

@tarruda

tarruda Dec 14, 2017

Author Contributor

What are the two calls that happen prior to adding the listener?

As I explained to @alexeykuzmin:

I also added an assertion in the before callback to make it explicit that getCalls returns two calls prior to adding the listener.

If we keep the preventDefault approach, please add a test here to exercise it

I don't see how I could test this since there's no way of checking if a file descriptor was closed. (python-dbusmock doesn't even send a real file descriptor when it returns from an Inhibit request)

// Notification that the system is rebooting or shutting down. If the
// implementation returns true, the PowerObserver instance should try to delay
// OS shutdown so the application can perform cleanup before exiting.
virtual bool OnShutdown() { return false; }

This comment has been minimized.

@ckerr

ckerr Dec 13, 2017

Member

This code only works if the caller and the called are the same object (an instance of PowerMonitor inheriting from PowerObserver).. It's kind of a template method via a crazy dependency between observer and monitor. Likewise, this is why Observer has new verb methods in this PR.

Mind you, I understand the choice -- we've already got the DBus code in Observer, so unless we refactor the DBus code into another place (eg a self-contained class or in PowerMonitorLinux) then Monitor has to have these Observer hooks.

This is going to be a pain to test, debug, and maintain in the future and I'd love it if we could somehow clean up the class' roles in in this PR. That's a big ask though.

This comment has been minimized.

@tarruda

tarruda Dec 14, 2017

Author Contributor

I'm not sure I understand your request as I'm not very skilled in C++ design patterns. Can you give an example of what you have in mind?

@tarruda

This comment has been minimized.

Copy link
Contributor Author

tarruda commented Dec 14, 2017

This seems like unnecessary hoops to jump through. I'd vote for removing this js bootstrap code and just auto-inhibiting in the C++ code, same as we do now for sleep.

The reason I enabled inhibiting only when a listener is added is because it would cause a small overhead when the OS shutdown even when no listener is added (since systemd has to wait until the ScopedFD is closed, which only happens when the app runs the JS event handler). I imagine there are many electron apps that don't care about cleanup code, it would be a shame if these apps caused unnecessary delays during shutdown.

I was planning on doing the same optimization for suspend event in a new PR after this one. In other words, Inhibit would only be called for sleep after a listener is added to suspend event.

I can leave as it is to reduce extra code, but IMHO it would not be good to make every electron app to add these inhibit locks unconditionally.

PrepareForShutdown and PrepareForSleep have the same arguments and concepts, but we're handling one by releasing the lock immediately after emit and the other has a new custom handler. Is this to avoid breakage in how sleep is handled on macOS and Windows?

Basically yes. If we allowed the app to prevent suspend with e.preventDefault(), we would need another API to allow the suspend to continue, and I'm not sure that would map well to macOS/Windows.

@tarruda

This comment has been minimized.

Copy link
Contributor Author

tarruda commented Dec 14, 2017

And, I know I've been a big pain already, but please document the API in docs/api/power-monitor.md. Thank you! 😄

I was planning to write docs after finishing the code and addressing all reviews.

@tarruda tarruda force-pushed the power-monitor-shutdown-event-and-delay-api branch from e179fee to 99c6ba1 Dec 21, 2017

@tarruda tarruda requested a review from electron/docs as a code owner Dec 21, 2017

@tarruda

This comment has been minimized.

Copy link
Contributor Author

tarruda commented Dec 21, 2017

I believe I addressed or replied to all comments, please let me know if something was missed

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Feb 5, 2018

The API design matches macOS/Windows well, it would be straightforward to implement this API on macOS.

But the common implementation code does not fit other platforms' API designs, and most code can not be reused for other platforms, so I'm going to simplify the implementation by making certain code Linux only.

@zcbenz zcbenz force-pushed the power-monitor-shutdown-event-and-delay-api branch from 99c6ba1 to e0e7dd2 Feb 5, 2018

zcbenz added some commits Feb 5, 2018

Do not use virtual function to request shutdown
Would make it easier to port to other platforms.
Remove the AllocateSystemIOPorts call
It is no longer needed and it is crashing.
@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Feb 5, 2018

I have added implementation for macOS.

@zcbenz

zcbenz approved these changes Feb 5, 2018

@zcbenz zcbenz merged commit ab015e5 into master Feb 5, 2018

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test 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

@zcbenz zcbenz deleted the power-monitor-shutdown-event-and-delay-api branch Feb 5, 2018

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