Skip to content

[lldb-dap] Waiting for the test binary to exit prior to dumping logs. #131917

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.

Sign up for GitHub

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

Merged
ashgti merged 2 commits into from
Mar 19, 2025
Merged

[lldb-dap] Waiting for the test binary to exit prior to dumping logs. #131917

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca59812
[lldb-dap] Waiting for the test binary to exit prior to dumping logs.
ashgti Mar 18, 2025
36ceaeb
Adding a timeout to the waits to ensure we try to print some logs.
ashgti Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,13 @@ def packet_type_is(packet, packet_type):


def dump_dap_log(log_file):
print("========= DEBUG ADAPTER PROTOCOL LOGS =========")
print("========= DEBUG ADAPTER PROTOCOL LOGS =========", file=sys.stderr)
if log_file is None:
print("no log file available")
print("no log file available", file=sys.stderr)
else:
with open(log_file, "r") as file:
print(file.read())
print("========= END =========")
print(file.read(), file=sys.stderr)
print("========= END =========", file=sys.stderr)


def read_packet_thread(vs_comm, log_file):
Expand All @@ -107,6 +107,10 @@ def read_packet_thread(vs_comm, log_file):
# termination of lldb-dap and stop waiting for new packets.
done = not vs_comm.handle_recv_packet(packet)
finally:
# Wait for the process to fully exit before dumping the log file to
# ensure we have the entire log contents.
if vs_comm.process is not None:
vs_comm.process.wait()
Copy link
Member

@JDevlieghere JDevlieghere Mar 18, 2025

Choose a reason for hiding this comment

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

Should we provide a reasonable timeout (e.g. 5 seconds) so that if the process hangs, we still dump the logs? Maybe even send a kill?

Copy link
Member

@walter-erquinigo walter-erquinigo Mar 18, 2025

Choose a reason for hiding this comment

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

I'd say 20 seconds. Some build bots make every test run very slowly

Copy link
Member

@JDevlieghere JDevlieghere Mar 18, 2025

Choose a reason for hiding this comment

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

I'm fine with 20 (or really anything below 600, which is the individual lit timeout).

Copy link
Contributor Author

@ashgti ashgti Mar 18, 2025

Choose a reason for hiding this comment

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

The test itself usually handles this with the call to terminate in

llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

Line 1273 in b251c29

def terminate(self):

called from:

llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

Line 336 in b251c29

self.dap_server.terminate()

But the read_packet_thread thread is never joined or waited on so it tends to write the DAP logs whenever it thinks its done with the connection, not when the process has exited.

Copy link
Member

@JDevlieghere JDevlieghere Mar 18, 2025

Choose a reason for hiding this comment

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

Okay, so the main thread is going to send a SIGTERM asking the process to terminate gracefully. I think the timeout still makes sense (at least here) in case it doesn't, but if we wanted to send a SIGKILL, I agree we should do it where we currently call terminate.

Copy link
Contributor Author

@ashgti ashgti Mar 19, 2025

Choose a reason for hiding this comment

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

Updated both of the waits to have a timeout of 20s.

For the read_packet_thread, we'll just print whatever logs we find after the timeout.

For the terminate call, we upgrade to a kill().

dump_dap_log(log_file)


Expand Down
Loading