fix: fix dde-session not started after ddm restart or crash recover #643
Conversation
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
This commit is boundled with a ddm update commit.
|
Hi @calsys456. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts D-Bus watching logic to reliably activate the compositor socket after ddm restart/crash, and enhances greeter crash-recovery handling by propagating session ID when a user is already logged in. Sequence diagram for compositor socket activation after ddm restartsequenceDiagram
participant SystemdSocket as systemd_socket_process
participant QDBus as qdbus_connection
participant Compositor as org_deepin_Compositor1
SystemdSocket->>QDBus: register QDBusServiceWatcher(Compositor1, WatchForOwnerChange)
SystemdSocket->>QDBus: connect SessionChanged signal to SignalReceiver(onSessionChanged)
SystemdSocket->>SystemdSocket: activateFd() (initial activation)
Note over Compositor,QDBus: ddm/compositor crashes and restarts
Compositor->>QDBus: acquire bus name org.deepin.Compositor1 (owner change)
QDBus-->>SystemdSocket: serviceOwnerChanged for org.deepin.Compositor1
SystemdSocket->>SystemdSocket: sleep(1) to wait for compositor readiness
SystemdSocket->>SystemdSocket: activateFd() (reactivate socket)
Compositor-->>QDBus: emit SessionChanged
QDBus-->>SystemdSocket: SessionChanged signal
SystemdSocket->>SystemdSocket: SignalReceiver.onSessionChanged()
SystemdSocket->>SystemdSocket: activateFd() (on session change)
Sequence diagram for greeter crash-recovery user login handlingsequenceDiagram
participant User as user
participant DDM as ddm_daemon
participant Greeter as GreeterProxy
participant UserModel as UserModel
participant Session as dde_session
Note over User,DDM: User is already logged in before crash
Note over DDM,Greeter: After crash recovery, ddm reconnects to greeter
DDM->>Greeter: DaemonMessages.UserLoggedIn(user, sessionId)
Greeter->>Greeter: readyRead() parses user and sessionId
Greeter->>UserModel: getUser(user)
UserModel-->>Greeter: userPtr
alt user found
Greeter->>UserModel: userPtr.setLogined(true)
Greeter->>UserModel: emit userLoggedIn(user, sessionId)
UserModel->>Session: re-associate greeter with existing sessionId
else user not found
Greeter->>Greeter: log warning user not found
end
Class diagram for updated greeter login handlingclassDiagram
class GreeterProxy {
+void readyRead()
-void handleUserLoggedIn(QString user, int sessionId)
}
class UserModel {
+shared_ptr<User> getUser(QString name)
+void userLoggedIn(QString user, int sessionId)
}
class User {
+void setLogined(bool logined)
}
GreeterProxy --> UserModel : uses
UserModel --> User : manages
UserModel : signal userLoggedIn(QString user, int sessionId)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Collaborator
Author
|
Related with linuxdeepin/ddm#54 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Blocking the main thread with
sleep(1)inside theserviceOwnerChangedlambda can delay all event processing; consider using aQTimer::singleShotto deferactivateFd()without blocking. - Now that you're watching
WatchForOwnerChange, the slot will be invoked for both appearance and disappearance of the service; if you only want to react on (re)registration, consider checking thenewOwnerparameter in theserviceOwnerChangedhandler before callingactivateFd().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Blocking the main thread with `sleep(1)` inside the `serviceOwnerChanged` lambda can delay all event processing; consider using a `QTimer::singleShot` to defer `activateFd()` without blocking.
- Now that you're watching `WatchForOwnerChange`, the slot will be invoked for both appearance and disappearance of the service; if you only want to react on (re)registration, consider checking the `newOwner` parameter in the `serviceOwnerChanged` handler before calling `activateFd()`.
## Individual Comments
### Comment 1
<location> `src/systemd-socket.cpp:154` </location>
<code_context>
- activateFd);
-
- // Listen to SessionChanged signal and activate fd when session changed
+ [&] {
+ sleep(1); // wait for compositor to be ready
+ activateFd();
</code_context>
<issue_to_address>
**issue (bug_risk):** The lambda signature is incompatible with QDBusServiceWatcher::serviceOwnerChanged
`serviceOwnerChanged` emits three `QString` arguments (`service`, `oldOwner`, `newOwner`), but this lambda takes none, so its signature is incompatible and the `connect` will not compile or work correctly. Please update the lambda to accept the three parameters (even if unused), e.g. `[&](const QString &, const QString &, const QString &) { ... }`.
</issue_to_address>
### Comment 2
<location> `src/systemd-socket.cpp:155` </location>
<code_context>
-
- // Listen to SessionChanged signal and activate fd when session changed
+ [&] {
+ sleep(1); // wait for compositor to be ready
+ activateFd();
+ });
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Blocking `sleep(1)` in the signal handler can stall the event loop
`sleep(1)` here blocks the thread processing DBus signals (likely the main event loop), freezing UI and delaying other handlers. Use a non-blocking delay, e.g. `QTimer::singleShot(1000, this, &::activateFd);`, so the event loop stays responsive.
Suggested implementation:
```cpp
QObject::connect(compositorWatcher,
&QDBusServiceWatcher::serviceOwnerChanged,
compositorWatcher,
[&] {
// Use a non-blocking delay so the DBus/event loop thread stays responsive
QTimer::singleShot(1000, [&] {
activateFd();
});
});
```
1. Ensure the file includes the QTimer header near the top of `src/systemd-socket.cpp`:
- `#include <QTimer>`
2. If your project uses forward declarations or a precompiled header for Qt widgets/core, make sure QTimer is available there instead of (or in addition to) a local include.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zccrs
requested changes
Dec 12, 2025
zccrs
approved these changes
Dec 12, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit is boundled with a ddm update commit.
Summary by Sourcery
Ensure dde-session is correctly reactivated when the compositor restarts or recovers from a crash and propagate existing user login state with session identifiers after daemon recovery.
Bug Fixes: