Skip to content

fix: can`t handle terminated event from debugpy #1119

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LiHua000
Copy link
Contributor

@LiHua000 LiHua000 commented May 14, 2025

Log: cppdap does not process event which body is empty, but 'body'is an optional filed, it should handle such cases.

Bug:

Summary by Sourcery

Allow processing of DAP events with empty bodies and remove obsolete thread-based termination logic.

Bug Fixes:

  • Whitelist the "terminated" event so cppdap handles it even when its body is empty.
  • Remove thread tracking in DAPDebugger that blocked termination detection for events with empty bodies.

Enhancements:

  • Add include to support event whitelist.

Log: cppdap does not process event which body is empty, but 'body'is an optional filed, it should handle such cases.

Bug:
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LiHua000
Once this PR has been reviewed and has the lgtm label, please assign toberyan for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

sourcery-ai bot commented May 14, 2025

Reviewer's Guide

This PR removes manual thread tracking and termination handling from the DAP debugger and updates the DAP session deserializer to gracefully handle empty bodies for specific events (notably "terminated").

File-Level Changes

Change Details Files
Removed manual thread tracking and termination handling logic in DAPDebugger
  • Deleted the threads member and its clear call in exitDebug()
  • Removed ThreadEvent handlers for "started" and "exited" reasons
  • Eliminated printing of termination output and run state updates
src/plugins/debugger/dap/dapdebugger.cpp
Allow empty bodies for specific DAP events during deserialization
  • Added #include
  • Introduced bodyCanBeEmpty set containing "terminated"
  • Adjusted deserialize callback to skip body_ok=false for allowed events
3rdparty/cppdap/src/session.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @LiHua000 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@@ -418,7 +419,9 @@ class Impl : public dap::Session {
// "body" is an optional field for some events, such as "Terminated Event".
bool body_ok = true;
d->field("body", [&](dap::Deserializer* d) {
if (!typeinfo->deserialize(d, data)) {
// todo: to completed
std::set<std::string> bodyCanBeEmpty { "terminated" };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Optimize optional-body lookup and broaden handling

Make bodyCanBeEmpty a static const to avoid reallocations, and review the DAP spec for other events (e.g. initialized, stopped) that may omit a body to prevent false negatives.

@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. session.cpp文件中,新增了#include <set>,但是没有看到对应的using namespace std;声明,建议在文件顶部添加以避免命名空间冲突。

  2. session.cpp文件中,bodyCanBeEmpty集合被定义,但是没有看到对应的注释说明其用途,建议添加注释说明集合的作用。

  3. appoutputpane.cpp文件中,setProcessStarted函数中使用了d->appIsRunning.contains(id),但是d->appIsRunning的类型是QMap<QString, bool>,应该使用contains方法来检查键是否存在。

  4. dapdebugger.cpp文件中,startDebug函数中使用了QMetaObject::invokeMethod来异步调用方法,但是没有看到对应的错误处理机制,建议添加错误处理代码。

  5. dapdebugger.cpp文件中,registerDapHandlers函数中删除了处理ThreadEvent的代码,但是没有提供替代的实现,可能会导致功能缺失。

  6. dapdebugger.cpp文件中,exitDebug函数中删除了d->threads.clear();,但是没有提供替代的实现,可能会导致线程信息丢失。

  7. dapdebugger.cpp文件中,launchSession函数中删除了创建调试面板的代码,但是没有提供替代的实现,可能会导致调试面板无法创建。

  8. dapdebugger.cpp文件中,launchSession函数中添加了AppOutputPane::instance()->setProcessStarted("debugPane");,但是没有看到对应的注释说明其用途,建议添加注释说明代码的作用。

  9. dapdebugger.h文件中,DAPDebugger类的声明中,dpfservice::ProjectInfo getActiveProjectInfo() const;函数的注释说明不够详细,建议添加更详细的注释说明函数的用途和返回值。

以上是本次代码审查的改进意见,希望能够对您有所帮助。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants