-
Notifications
You must be signed in to change notification settings - Fork 30
fix: avoid memory leak #157
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
Conversation
Reviewer's GuideThis PR attaches heap-allocated Qt objects to the application’s parent, refactors the Fifo class for QObject ownership, and adds explicit cleanup to prevent leaks. Class diagram for updated Fifo ownership and constructionclassDiagram
class Fifo {
+Fifo(QObject *parent = nullptr)
+virtual ~Fifo()
+int Read(QString &data)
+int Write(QString data)
-int m_fd
-QString m_fifoPath
}
Fifo --|> QObject
Class diagram for updated Session and WMSwitcher constructionclassDiagram
class Session {
+Session(QObject *parent = nullptr)
}
Session --|> QObject
class WMSwitcher {
+WMSwitcher(QObject *parent = nullptr)
}
WMSwitcher --|> QObject
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @yixinshark - I've reviewed your changes - here's some feedback:
- You removed the background thread for the write‐FIFO and now invoke OpenWrite/Write on the main thread—this can block your UI, so consider keeping it in a worker thread or making it nonblocking.
- Calling deleteLater() on the FIFO inside a QThreadPool task won’t free it if there’s no running event loop—either give it a QObject parent or delete it explicitly when done.
- Make sure Fifo’s destructor properly closes m_fd and cleans up the FIFO file to avoid resource leaks at teardown.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You removed the background thread for the write‐FIFO and now invoke OpenWrite/Write on the main thread—this can block your UI, so consider keeping it in a worker thread or making it nonblocking.
- Calling deleteLater() on the FIFO inside a QThreadPool task won’t free it if there’s no running event loop—either give it a QObject parent or delete it explicitly when done.
- Make sure Fifo’s destructor properly closes m_fd and cleans up the FIFO file to avoid resource leaks at teardown.
## Individual Comments
### Comment 1
<location> `src/dde-session/main.cpp:103` </location>
<code_context>
- qInfo()<<"leader pipe thread id: " << QThread::currentThreadId() << ", pid: " << curPid;
- Fifo *fifo = new Fifo;
+
+ { // none block
+ pid_t curPid = getpid();
+ Fifo *fifo = new Fifo(&app);
fifo->OpenWrite();
fifo->Write(QString::number(curPid));
- });
-
+ }
// We started the unit, open <dbus> and sleep forever.
return app.exec();
</code_context>
<issue_to_address>
Switching from QThreadPool to direct execution for Fifo write may affect responsiveness.
Since Fifo writes are now synchronous, any delays or blocking in Fifo::OpenWrite or Fifo::Write could negatively impact startup or UI responsiveness. Please confirm this change is intentional and safe across all environments.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/dde-session/main.cpp
Outdated
| { // none block | ||
| pid_t curPid = getpid(); | ||
| Fifo *fifo = new Fifo(&app); | ||
| fifo->OpenWrite(); | ||
| fifo->Write(QString::number(curPid)); | ||
| }); | ||
|
|
||
| } |
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.
issue (performance): Switching from QThreadPool to direct execution for Fifo write may affect responsiveness.
Since Fifo writes are now synchronous, any delays or blocking in Fifo::OpenWrite or Fifo::Write could negatively impact startup or UI responsiveness. Please confirm this change is intentional and safe across all environments.
c82a45d to
618ed88
Compare
as title Log: as title Pms: BUG-318023
618ed88 to
820cffc
Compare
deepin pr auto review关键摘要:
是否建议立即修改: |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, yixinshark 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 |
as title
Log: as title
Pms: BUG-318023
Summary by Sourcery
Ensure proper Qt object ownership and cleanup to eliminate memory leaks in dde-session by parenting dynamically allocated objects and disposing of FIFO instances after use.
Bug Fixes:
Enhancements: