[debugging] Retry JDWP process lookup - #12324
Open
jonathanpeppers wants to merge 4 commits into
Open
Conversation
Retry transient PID discovery with bounded exponential backoff and propagate JDWP attachment failures so RunActivity cannot report success while the app remains suspended. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use the existing fixed-delay PID lookup helper for a bounded five-second retry window instead of adding exponential-backoff plumbing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f60a857e-d77c-48c4-ac14-f9bee00159ab
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f60a857e-d77c-48c4-ac14-f9bee00159ab
Exercise eventual PID discovery, bounded exhaustion, and cancellation through an internal injectable process lookup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f60a857e-d77c-48c4-ac14-f9bee00159ab
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability of Java debugging startup in the Android tooling by retrying JDWP process discovery for a bounded window, honoring cancellation while waiting, and letting terminal JDWP failures surface as MSBuild task failures (instead of being logged as a warning and continuing).
Changes:
- Extend PID lookup retries to allow up to ~5 seconds for the process to appear before attempting JDWP attach.
- Propagate JDWP connection failures (including “process not found”) instead of swallowing them as a warning.
- Add focused unit tests for retry success, retry exhaustion, and cancellation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Xamarin.Android.Tools.AndroidSdk-Tests/Xamarin.Android.Tools.AndroidSdk-Tests.csproj | Adds a project reference needed by the new unit tests. |
| tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidDeviceExtensionsTests.cs | New tests covering PID lookup retry behavior and cancellation. |
| src/Xamarin.AndroidTools/Devices/AndroidDeviceExtensions.cs | Refactors PID lookup to allow injection for testing and adds cancellation-aware delay. |
| src/Xamarin.AndroidTools/Debugging/DebuggingExtensions.cs | Extends JDWP PID lookup window, stops swallowing JDWP failures, ensures cleanup of JDWP forwarding/client. |
| src/Xamarin.AndroidTools/AssemblyInfo.cs | Grants test assembly access to internal helper for unit testing. |
Comment on lines
+180
to
+183
| using (var jdwpClient = new JdwpClient (config.Debugger.JdwpHostName, config.Debugger.JdwpPort)) { | ||
| await AdbServer.Default.ForwardPort (androidDevice, "tcp", jdwpClient.Port, "jdwp", pid, token); | ||
| try { | ||
| await jdwpClient.ConnectAsync (token); |
Comment on lines
146
to
+149
| public static async Task<int> GetProcessIDAsync(this AndroidDevice device, string packageName, int maxAttempts, int timeBetweenAttempts, CancellationToken token) | ||
| { | ||
| return await GetProcessIDAsync (cancellationToken => device.GetProcessId (packageName, cancellationToken), maxAttempts, timeBetweenAttempts, token); | ||
| } |
Comment on lines
+27
to
+44
| [Test] | ||
| public async Task GetProcessIDAsyncStopsAfterMaximumAttempts () | ||
| { | ||
| var attempts = 0; | ||
|
|
||
| var processId = await AndroidDeviceExtensions.GetProcessIDAsync ( | ||
| _ => { | ||
| attempts++; | ||
| return Task.FromResult (0); | ||
| }, | ||
| maxAttempts: 2, | ||
| timeBetweenAttempts: 0, | ||
| token: CancellationToken.None | ||
| ); | ||
|
|
||
| Assert.AreEqual (0, processId); | ||
| Assert.AreEqual (3, attempts); | ||
| } |
Comment on lines
+52
to
+57
| Assert.ThrowsAsync<TaskCanceledException> (() => AndroidDeviceExtensions.GetProcessIDAsync ( | ||
| _ => Task.FromResult (0), | ||
| maxAttempts: 20, | ||
| timeBetweenAttempts: 1, | ||
| token: cancellationTokenSource.Token | ||
| )); |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Pull Request
title and
description
should follow the
commit-messages.mdworkflow documentation, and in particular should include:RunActivitycan start an emulator app suspended for JDWP before the process appears inadbprocess discovery. The previous short lookup window then logged JDWP attachment failure as a warning and returned success, leaving the app suspended and causing the managed debug session to exit without an actionable build error.Extend the existing fixed-delay PID lookup to a bounded five-second window, propagate terminal JDWP failures to MSBuild, honor cancellation during waits, and reliably dispose the JDWP client and remove its ADB forward. Focused tests cover eventual process discovery, retry exhaustion, and cancellation.
Fixes #12294