Skip to content

Commit 9d191e1

Browse files
andrewlockandrewlock
authored
Fix potential deadlocks in IisFixture process code (#7281)
## Summary of changes Fixes potential deadlocks in the IIS fixture code ## Reason for change While working on the .NET branch, we had to update xunit. We subsequently started seeing deadlocks in the Iis tests. I noticed potential issues with the code that reads the process output (because I fixed something similar in #7116 recently) and voila, no deadlocks 🎉 ## Implementation details Use the event-based handling already available in `ProcessHelper` instead of spawning separate dedicated threads for handling reading the output. Also some minor additional things: - Allowed specifying a time to wait after calling `Process.Kill()` in `ProcessHelper` before returning (we need this for the IIS Fixture) - ~Make sure we dispose the `Process` object held by `ProcessHelper`~ This _is_ already disposed, because it gets "owned" by the `ProcessResult` object instead ## Test coverage Covered by existing tests - if it passes, we're good ## Other details Following are dependent on this PR: - #7170 - #7280 https://datadoghq.atlassian.net/browse/LANGPLAT-632
1 parent 528a647 commit 9d191e1

File tree

5 files changed

+33
-42
lines changed

5 files changed

+33
-42
lines changed

‎tracer/test/Datadog.Trace.TestHelpers.AutoInstrumentation/IisFixture.cs

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ namespace Datadog.Trace.TestHelpers
1515
[CollectionDefinition("IisTests", DisableParallelization = false)]
1616
public sealed class IisFixture : GacFixture, IDisposable
1717
{
18-
public (Process Process, string ConfigFile) IisExpress { get; private set; }
18+
public (ProcessHelper Process, string ConfigFile) IisExpress { get; private set; }
1919

2020
public MockTracerAgent Agent { get; private set; }
2121

@@ -65,22 +65,19 @@ public void Dispose()
6565
{
6666
try
6767
{
68-
if (!IisExpress.Process.HasExited)
69-
{
70-
// sending "Q" to standard input does not work because
71-
// iisexpress is scanning console key press, so just kill it.
72-
// maybe try this in the future:
73-
// https://github.com/roryprimrose/Headless/blob/master/Headless.IntegrationTests/IisExpress.cs
74-
IisExpress.Process.Kill();
75-
IisExpress.Process.WaitForExit(8000);
76-
}
68+
// sending "Q" to standard input does not work because
69+
// iisexpress is scanning console key press, so just kill it.
70+
// maybe try this in the future:
71+
// https://github.com/roryprimrose/Headless/blob/master/Headless.IntegrationTests/IisExpress.cs
72+
IisExpress.Process.Dispose(8000);
7773
}
7874
catch
7975
{
8076
// in some circumstances the HasExited property throws, this means the process probably hasn't even started correctly
8177
}
8278

83-
IisExpress.Process.Dispose();
79+
// The ProcessHelper doesn't dispose the process automatically, so we need to do it manually
80+
IisExpress.Process.Process.Dispose();
8481

8582
try
8683
{

‎tracer/test/Datadog.Trace.TestHelpers.AutoInstrumentation/TestHelper.cs

Lines changed: 9 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ public ProcessResult WaitForProcessResult(ProcessHelper helper, int expectedExit
213213
return new ProcessResult(process, standardOutput, standardError, exitCode);
214214
}
215215

216-
public async Task<(Process Process, string ConfigFile)> StartIISExpress(MockTracerAgent agent, int iisPort, IisAppType appType, string subAppPath)
216+
public async Task<(ProcessHelper Process, string ConfigFile)> StartIISExpress(MockTracerAgent agent, int iisPort, IisAppType appType, string subAppPath)
217217
{
218218
var iisExpress = EnvironmentHelper.GetIisExpressPath();
219219

@@ -285,30 +285,18 @@ public ProcessResult WaitForProcessResult(ProcessHelper helper, int expectedExit
285285

286286
var semaphore = new SemaphoreSlim(0, 1);
287287

288-
_ = Task.Factory.StartNew(
289-
() =>
288+
var processHelper = new ProcessHelper(
289+
process,
290+
line =>
290291
{
291-
while (process.StandardOutput.ReadLine() is { } line)
292-
{
293-
Output.WriteLine($"[webserver][stdout] {line}");
292+
Output.WriteLine($"[webserver][stdout] {line}");
294293

295-
if (line.Contains("IIS Express is running"))
296-
{
297-
semaphore.Release();
298-
}
299-
}
300-
},
301-
TaskCreationOptions.LongRunning);
302-
303-
_ = Task.Factory.StartNew(
304-
() =>
305-
{
306-
while (process.StandardError.ReadLine() is { } line)
294+
if (line.Contains("IIS Express is running"))
307295
{
308-
Output.WriteLine($"[webserver][stderr] {line}");
296+
semaphore.Release();
309297
}
310298
},
311-
TaskCreationOptions.LongRunning);
299+
line => Output.WriteLine($"[webserver][stderr] {line}"));
312300

313301
await semaphore.WaitAsync(TimeSpan.FromSeconds(10));
314302

@@ -335,7 +323,7 @@ public ProcessResult WaitForProcessResult(ProcessHelper helper, int expectedExit
335323
await Task.Delay(1500);
336324
}
337325

338-
return (process, newConfig);
326+
return (processHelper, newConfig);
339327
}
340328

341329
public void EnableIast(bool enable = true)

‎tracer/test/Datadog.Trace.TestHelpers/ProcessHelper.cs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,13 +111,19 @@ public bool Drain(int timeout = Timeout.Infinite)
111111
&& _errorTask.Task.Wait(timeout);
112112
}
113113

114-
public virtual void Dispose()
114+
public virtual void Dispose() => Dispose(0);
115+
116+
public virtual void Dispose(int waitForExitTimeout)
115117
{
116118
if (!Process.HasExited)
117119
{
118120
try
119121
{
120122
Process.Kill();
123+
if (waitForExitTimeout > 0)
124+
{
125+
Process.WaitForExit(waitForExitTimeout);
126+
}
121127
}
122128
catch
123129
{

‎tracer/test/Datadog.Trace.Tools.dd_dotnet.ArtifactTests/Checks/IisCheckTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ private static string GetSampleProjectPath()
187187

188188
private static string IisExpressOptions(IisFixture iisFixture)
189189
{
190-
return $"--workerProcess {iisFixture.IisExpress.Process.Id} --iisConfigPath {iisFixture.IisExpress.ConfigFile}";
190+
return $"--workerProcess {iisFixture.IisExpress.Process.Process.Id} --iisConfigPath {iisFixture.IisExpress.ConfigFile}";
191191
}
192192

193193
private static void EnsureWindowsAndX64()

‎tracer/test/Datadog.Trace.Tools.dd_dotnet.IntegrationTests/Checks/IisCheckTests.cs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public async Task WorkingApp(bool mixedRuntimes)
5353
var result = await CheckIisCommand.ExecuteAsync(
5454
siteName,
5555
iisFixture.IisExpress.ConfigFile,
56-
iisFixture.IisExpress.Process.Id,
56+
iisFixture.IisExpress.Process.Process.Id,
5757
MockRegistryService());
5858

5959
result.Should().Be(0);
@@ -93,7 +93,7 @@ public async Task NestedApp()
9393
var result = await CheckIisCommand.ExecuteAsync(
9494
siteName,
9595
iisFixture.IisExpress.ConfigFile,
96-
iisFixture.IisExpress.Process.Id,
96+
iisFixture.IisExpress.Process.Process.Id,
9797
MockRegistryService());
9898

9999
result.Should().Be(0);
@@ -126,7 +126,7 @@ public async Task OutOfProcess()
126126
var result = await CheckIisCommand.ExecuteAsync(
127127
"sample",
128128
iisFixture.IisExpress.ConfigFile,
129-
iisFixture.IisExpress.Process.Id,
129+
iisFixture.IisExpress.Process.Process.Id,
130130
MockRegistryService());
131131

132132
result.Should().Be(0);
@@ -162,7 +162,7 @@ public async Task OutOfProcessNotInitialized()
162162
var result = await CheckIisCommand.ExecuteAsync(
163163
"sample",
164164
iisFixture.IisExpress.ConfigFile,
165-
iisFixture.IisExpress.Process.Id,
165+
iisFixture.IisExpress.Process.Process.Id,
166166
MockRegistryService());
167167

168168
result.Should().Be(1);
@@ -188,7 +188,7 @@ public async Task NoGac()
188188
var result = await CheckIisCommand.ExecuteAsync(
189189
"sample",
190190
iisFixture.IisExpress.ConfigFile,
191-
iisFixture.IisExpress.Process.Id,
191+
iisFixture.IisExpress.Process.Process.Id,
192192
MockRegistryService());
193193

194194
result.Should().Be(1);
@@ -209,7 +209,7 @@ public async Task ListSites()
209209
var result = await CheckIisCommand.ExecuteAsync(
210210
"dummySite",
211211
iisFixture.IisExpress.ConfigFile,
212-
iisFixture.IisExpress.Process.Id,
212+
iisFixture.IisExpress.Process.Process.Id,
213213
MockRegistryService());
214214

215215
result.Should().Be(1);
@@ -230,7 +230,7 @@ public async Task ListApplications()
230230
var result = await CheckIisCommand.ExecuteAsync(
231231
"sample/dummy",
232232
iisFixture.IisExpress.ConfigFile,
233-
iisFixture.IisExpress.Process.Id,
233+
iisFixture.IisExpress.Process.Process.Id,
234234
MockRegistryService());
235235

236236
result.Should().Be(1);
@@ -251,7 +251,7 @@ public async Task IncorrectlyConfiguredAppPool()
251251
var result = await CheckIisCommand.ExecuteAsync(
252252
"sample",
253253
iisFixture.IisExpress.ConfigFile,
254-
iisFixture.IisExpress.Process.Id,
254+
iisFixture.IisExpress.Process.Process.Id,
255255
MockRegistryService());
256256

257257
result.Should().Be(1);

0 commit comments

Comments
 (0)