-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
systemd: Don't open and close tuned DBus connections asynchronously #21114
base: main
Are you sure you want to change the base?
systemd: Don't open and close tuned DBus connections asynchronously #21114
Conversation
Starting the service might be slow and it would be best to do it inside the dialog, while the spinner is showing. That is probably straightforward. Also, I think our DBusClient is supposed to survive the dbus service coming and going, maybe with some flags and owner watching. That would be another, even cleaner, way of handling this, by only having a single DBusClient that is never closed and always works. |
(except when superuser.allowed changes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, the dialog loads a lot faster!
@@ -303,14 +300,14 @@ const TunedDialog = ({ | |||
className="ct-m-stretch-body" | |||
isOpen | |||
help={help} | |||
onClose={Dialogs.close} | |||
onClose={() => dialogResult.resolve()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equivalent would be dialogResult.resolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily. The this
inside the call is different, and that might matter.
Done in #21115. |
This is due to this: https://github.com/cockpit-project/cockpit/pull/21114/files#diff-edcd27aeefd91df68948709cd630aaefbc569ac0674cc54693139880404e17bdR101-R108 |
The TunedPerformanceProfile component was keeping a DBus connection open to tuned, and was closing and reopening it whenever something might have changed that requires it. This happened asynchronously with the rest of the code that was using this DBus connection, and would frequently cause that code to fail with "disconnected" errors. Also, the TunedDialog was itself causing changes that require a new DBus connection (namely starting the tuned.service), but was always working with the one that was originally passed into it via its properties. Let's straighten this all out: - Polling for the button state will open its own dedicated DBUs connection and close it when polling is done. There is no need to keep a DBus connection open for the whole Cockpit session. - The tuned.service is started before the dialog is opened and a DBus connection is opened explicitly for the dialog and closed when the dialog is closed.
ec42cb7
to
50c64f6
Compare
if (!tunedService.exists) | ||
return ({ state: "not-installed" }); | ||
else if (tunedService.state != "running") | ||
return ({ state: "not-running" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
else if (tunedService.state != "running") | ||
return ({ state: "not-running" }); | ||
else | ||
return Promise.reject(ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
} catch (ex) { | ||
console.warn("failed to poll tuned", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 added lines are not executed by any test.
setBtnText("error"); | ||
setStatus(_("Communication with tuned has failed")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 added lines are not executed by any test.
@@ -303,14 +300,14 @@ const TunedDialog = ({ | |||
className="ct-m-stretch-body" | |||
isOpen | |||
help={help} | |||
onClose={Dialogs.close} | |||
onClose={() => dialogResult.resolve()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
title={_("Change performance profile")} | ||
footer={ | ||
<> | ||
<Button variant='primary' isDisabled={!selected} onClick={setProfile}> | ||
{_("Change profile")} | ||
</Button> | ||
<Button variant='link' onClick={Dialogs.close}> | ||
<Button variant='link' onClick={() => dialogResult.resolve()}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
@jelly, what do you think about this? Should we start tuned.service while the dialog is already open? Then the TunedDialog component needs to maintain its own DBusClient ins a |
Hmmm right, what if it fails the error message get's "lost", so maybe we should do the extra work (and right thing) And start the service in the dialog? |
The TunedPerformanceProfile component was keeping a DBus connection open to tuned, and was closing and reopening it whenever something might have changed that requires it.
This happened asynchronously with the rest of the code that was using this DBus connection, and would frequently cause that code to fail with "disconnected" errors.
Also, the TunedDialog was itself causing changes that require a new DBus connection (namely starting the tuned.service), but was always working with the one that was originally passed into it via its properties.
Let's straighten this all out:
Polling for the button state will open its own dedicated DBUs connection and close it when polling is done. There is no need to keep a DBus connection open for the whole Cockpit session.
The tuned.service is started before the dialog is opened and a DBus connection is opened explicitly for the dialog and closed when the dialog is closed.