fix: fix some modules loaded failed #833
Merged
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
dde-session-daemon part2 load after org.deepin.dde.SessionManager1 running Log: pms: BUG-323803
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fly602 - I've reviewed your changes - here's some feedback:
- You’re ignoring the error from dbus.SessionBus()—make sure to handle or log it before using sessionBus to avoid nil-pointer issues.
- The signal loop started via sessionSigLoop.Start() is never stopped—add proper cleanup (e.g. sessionSigLoop.Stop()) after removing the handler to prevent resource leaks.
- Dropping the DDE_SESSION_PROCESS_COOKIE_ID check in StartPart2 may introduce unexpected states—verify that removing this validation is safe for all launch scenarios.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’re ignoring the error from dbus.SessionBus()—make sure to handle or log it before using sessionBus to avoid nil-pointer issues.
- The signal loop started via sessionSigLoop.Start() is never stopped—add proper cleanup (e.g. sessionSigLoop.Stop()) after removing the handler to prevent resource leaks.
- Dropping the DDE_SESSION_PROCESS_COOKIE_ID check in StartPart2 may introduce unexpected states—verify that removing this validation is safe for all launch scenarios.
## Individual Comments
### Comment 1
<location> `bin/dde-session-daemon/daemon.go:255` </location>
<code_context>
- } else {
- err = loader.EnableModules(s.getAllDefaultEnabledModules(),
- s.getAllDefaultDisabledModules(), getEnableFlag(s.flags))
+ err = loader.EnableModules(s.part1EnabledModules, s.part1DisabledModules, 0)
+ if err != nil {
+ logger.Warning("Failed to enable part1 modules:", err)
</code_context>
<issue_to_address>
Consider extracting the DBus and signal loop logic from execDefaultAction into a dedicated helper to keep the function focused and readable.
```suggestion
The new DBus/signal‐loop logic in `execDefaultAction` is adding a lot of noise. You can pull it into a small helper (e.g. `sessionWatcher`) and keep `execDefaultAction` focused:
--- session_watcher.go ---
```go
package main
import (
"sync"
"github.com/godbus/dbus/v5"
dbusmgr "github.com/linuxdeepin/go-dbus-factory/system/org.freedesktop.dbus"
"github.com/linuxdeepin/go-lib/dbusutil"
)
type sessionWatcher struct {
once sync.Once
mgr *dbusmgr.DBus
loop *dbusutil.SignalLoop
}
func newSessionWatcher(conn *dbus.Conn) *sessionWatcher {
loop := dbusutil.NewSignalLoop(conn, 10)
loop.Start()
mgr := dbusmgr.NewDBus(conn)
mgr.InitSignalExt(loop, true)
return &sessionWatcher{mgr: mgr, loop: loop}
}
// Start invokes `onStarted` once when the name appears (or already exists).
func (w *sessionWatcher) Start(onStarted func()) error {
handler, err := w.mgr.ConnectNameOwnerChanged(func(name, _, newOwner string) {
if name == "org.deepin.dde.SessionManager1" && newOwner != "" {
w.once.Do(func() {
onStarted()
w.mgr.RemoveHandler(handler)
})
}
})
if err != nil {
return err
}
ok, err := w.mgr.NameHasOwner(0, "org.deepin.dde.SessionManager1")
if err != nil {
return err
}
if ok {
w.once.Do(func() {
onStarted()
w.mgr.RemoveHandler(handler)
})
}
return nil
}
```
Then simplify `execDefaultAction`:
```go
func (s *SessionDaemon) execDefaultAction() {
if err := loader.EnableModules(s.part1EnabledModules, s.part1DisabledModules, 0); err != nil {
logger.Warning("Failed to enable part1 modules:", err)
os.Exit(3)
}
conn, _ := dbus.SessionBus()
watcher := newSessionWatcher(conn)
if err := watcher.Start(s.StartPart2); err != nil {
logger.Warning("session watcher setup failed:", err)
os.Exit(3)
}
}
```
This keeps all functionality, reduces nesting in `execDefaultAction`, and moves DBus details into a focused helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } else { | ||
| err = loader.EnableModules(s.getAllDefaultEnabledModules(), | ||
| s.getAllDefaultDisabledModules(), getEnableFlag(s.flags)) | ||
| err = loader.EnableModules(s.part1EnabledModules, s.part1DisabledModules, 0) |
There was a problem hiding this comment.
issue (complexity): Consider extracting the DBus and signal loop logic from execDefaultAction into a dedicated helper to keep the function focused and readable.
Suggested change
| err = loader.EnableModules(s.part1EnabledModules, s.part1DisabledModules, 0) | |
| The new DBus/signal‐loop logic in `execDefaultAction` is adding a lot of noise. You can pull it into a small helper (e.g. `sessionWatcher`) and keep `execDefaultAction` focused: | |
| --- session_watcher.go --- | |
| ```go | |
| package main | |
| import ( | |
| "sync" | |
| "github.com/godbus/dbus/v5" | |
| dbusmgr "github.com/linuxdeepin/go-dbus-factory/system/org.freedesktop.dbus" | |
| "github.com/linuxdeepin/go-lib/dbusutil" | |
| ) | |
| type sessionWatcher struct { | |
| once sync.Once | |
| mgr *dbusmgr.DBus | |
| loop *dbusutil.SignalLoop | |
| } | |
| func newSessionWatcher(conn *dbus.Conn) *sessionWatcher { | |
| loop := dbusutil.NewSignalLoop(conn, 10) | |
| loop.Start() | |
| mgr := dbusmgr.NewDBus(conn) | |
| mgr.InitSignalExt(loop, true) | |
| return &sessionWatcher{mgr: mgr, loop: loop} | |
| } | |
| // Start invokes `onStarted` once when the name appears (or already exists). | |
| func (w *sessionWatcher) Start(onStarted func()) error { | |
| handler, err := w.mgr.ConnectNameOwnerChanged(func(name, _, newOwner string) { | |
| if name == "org.deepin.dde.SessionManager1" && newOwner != "" { | |
| w.once.Do(func() { | |
| onStarted() | |
| w.mgr.RemoveHandler(handler) | |
| }) | |
| } | |
| }) | |
| if err != nil { | |
| return err | |
| } | |
| ok, err := w.mgr.NameHasOwner(0, "org.deepin.dde.SessionManager1") | |
| if err != nil { | |
| return err | |
| } | |
| if ok { | |
| w.once.Do(func() { | |
| onStarted() | |
| w.mgr.RemoveHandler(handler) | |
| }) | |
| } | |
| return nil | |
| } |
Then simplify execDefaultAction:
func (s *SessionDaemon) execDefaultAction() {
if err := loader.EnableModules(s.part1EnabledModules, s.part1DisabledModules, 0); err != nil {
logger.Warning("Failed to enable part1 modules:", err)
os.Exit(3)
}
conn, _ := dbus.SessionBus()
watcher := newSessionWatcher(conn)
if err := watcher.Start(s.StartPart2); err != nil {
logger.Warning("session watcher setup failed:", err)
os.Exit(3)
}
}This keeps all functionality, reduces nesting in execDefaultAction, and moves DBus details into a focused helper.
Reviewer's GuideUnify the session daemon startup by removing the cookie-based gating, always enabling part1 modules with robust error handling, and implementing a DBus-based signal listener (with immediate owner check) to trigger part2 when the SessionManager is available. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
TAG Bot New tag: 6.1.43 |
deepin pr auto review关键摘要:
是否建议立即修改: |
robertkill
approved these changes
Jul 15, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602, robertkill 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.
dde-session-daemon part2 load after org.deepin.dde.SessionManager1 running
Log:
pms: BUG-323803
Summary by Sourcery
Defer part2 module initialization until the DDE SessionManager1 D-Bus service is available, remove the outdated cookie check, and add robust error handling for part1 module loading.
Bug Fixes:
Enhancements: