From 7e40818ad11adafde2fc766b4f6fa35033121069 Mon Sep 17 00:00:00 2001 From: "Mike Harder (from Dev Box)" Date: Thu, 25 Jul 2024 22:15:49 -0700 Subject: [PATCH 1/8] Only remove session once body has been written, to minimize probability client retries but test-proxy has already removed the session --- .../Azure.Sdk.Tools.TestProxy/RecordingHandler.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs index 0beb7fca889..aadb801afa2 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs @@ -468,15 +468,13 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming remove = bool.Parse(removeHeader); } - var match = session.Session.Lookup(entry, session.CustomMatcher ?? Matcher, session.AdditionalSanitizers.Count > 0 ? Sanitizers.Concat(session.AdditionalSanitizers) : Sanitizers, remove); + var match = session.Session.Lookup(entry, session.CustomMatcher ?? Matcher, session.AdditionalSanitizers.Count > 0 ? Sanitizers.Concat(session.AdditionalSanitizers) : Sanitizers, remove: false); foreach (ResponseTransform transform in Transforms.Concat(session.AdditionalTransforms)) { transform.Transform(incomingRequest, match); } - Interlocked.Increment(ref Startup.RequestsPlayedBack); - outgoingResponse.StatusCode = match.StatusCode; foreach (var header in match.Response.Headers) @@ -497,6 +495,14 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming await WriteBodyBytes(bodyData, session.PlaybackResponseTime, outgoingResponse); } + + Interlocked.Increment(ref Startup.RequestsPlayedBack); + + // Only remove session once body has been written, to minimize probability client retries but test-proxy has already removed the session + if (remove) + { + session.Session.Lookup(entry, session.CustomMatcher ?? Matcher, session.AdditionalSanitizers.Count > 0 ? Sanitizers.Concat(session.AdditionalSanitizers) : Sanitizers, remove: true); + } } public byte[][] GetBatches(byte[] bodyData, int batchCount) From 1d79631a686db6a18ab5327a849c202be6b566b3 Mon Sep 17 00:00:00 2001 From: "Mike Harder (from Dev Box)" Date: Thu, 25 Jul 2024 22:19:47 -0700 Subject: [PATCH 2/8] Defer reading header --- .../RecordingHandler.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs index aadb801afa2..46b0e770089 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs @@ -460,14 +460,7 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming var entry = (await CreateEntryAsync(incomingRequest).ConfigureAwait(false)).Item1; - // If request contains "x-recording-remove: false", then request is not removed from session after playback. - // Used by perf tests to play back the same request multiple times. - var remove = true; - if (incomingRequest.Headers.TryGetValue("x-recording-remove", out var removeHeader)) - { - remove = bool.Parse(removeHeader); - } - + // Session may be removed later, but only after response has been fully written var match = session.Session.Lookup(entry, session.CustomMatcher ?? Matcher, session.AdditionalSanitizers.Count > 0 ? Sanitizers.Concat(session.AdditionalSanitizers) : Sanitizers, remove: false); foreach (ResponseTransform transform in Transforms.Concat(session.AdditionalTransforms)) @@ -498,6 +491,15 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming Interlocked.Increment(ref Startup.RequestsPlayedBack); + var remove = true; + + // If request contains "x-recording-remove: false", then request is not removed from session after playback. + // Used by perf tests to play back the same request multiple times. + if (incomingRequest.Headers.TryGetValue("x-recording-remove", out var removeHeader)) + { + remove = bool.Parse(removeHeader); + } + // Only remove session once body has been written, to minimize probability client retries but test-proxy has already removed the session if (remove) { From 20d3f0002a028f640c45afd03ff0c8421b8e4207 Mon Sep 17 00:00:00 2001 From: "Mike Harder (from Dev Box)" Date: Thu, 25 Jul 2024 22:35:57 -0700 Subject: [PATCH 3/8] Minimize diff --- tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs index 46b0e770089..26fdddedab7 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs @@ -491,10 +491,9 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming Interlocked.Increment(ref Startup.RequestsPlayedBack); - var remove = true; - // If request contains "x-recording-remove: false", then request is not removed from session after playback. // Used by perf tests to play back the same request multiple times. + var remove = true; if (incomingRequest.Headers.TryGetValue("x-recording-remove", out var removeHeader)) { remove = bool.Parse(removeHeader); From 85384d4a17dc8b28886f7528118d4ccd29b77fee Mon Sep 17 00:00:00 2001 From: "Mike Harder (from Dev Box)" Date: Thu, 25 Jul 2024 22:38:12 -0700 Subject: [PATCH 4/8] Code cleanup --- .../Azure.Sdk.Tools.TestProxy/RecordingHandler.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs index fb704a0f3c7..5e033feaa7a 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs @@ -506,18 +506,19 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming Interlocked.Increment(ref Startup.RequestsPlayedBack); + // Only remove session once body has been written, to minimize probability client retries but test-proxy has already removed the session + var remove = true; + // If request contains "x-recording-remove: false", then request is not removed from session after playback. // Used by perf tests to play back the same request multiple times. - var remove = true; if (incomingRequest.Headers.TryGetValue("x-recording-remove", out var removeHeader)) { remove = bool.Parse(removeHeader); } - // Only remove session once body has been written, to minimize probability client retries but test-proxy has already removed the session if (remove) { - session.Session.Lookup(entry, session.CustomMatcher ?? Matcher, sanitizer, remove: true); + session.Session.Lookup(entry, session.CustomMatcher ?? Matcher, sanitizers, remove: true); } } From 92b26acfbe853d61e69bfea69c2fa40d5164c406 Mon Sep 17 00:00:00 2001 From: Scott Beddall Date: Fri, 26 Jul 2024 10:44:12 -0700 Subject: [PATCH 5/8] remove the necessity of always locking to check if a session is sanitized --- .../RecordingHandler.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs index 5e033feaa7a..08511273dd8 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs @@ -457,16 +457,19 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming var sanitizers = SanitizerRegistry.GetSanitizers(session); - // we don't need to re-sanitize with recording-applicable sanitizers every time. just the very first one - lock (session) + if (!session.IsSanitized) { - if (!session.IsSanitized) + // we don't need to re-sanitize with recording-applicable sanitizers every time. just the very first one + lock (session) { - session.IsSanitized = true; - - foreach (RecordedTestSanitizer sanitizer in sanitizers) + if (!session.IsSanitized) { - session.Session.Sanitize(sanitizer); + foreach (RecordedTestSanitizer sanitizer in sanitizers) + { + session.Session.Sanitize(sanitizer); + } + + session.IsSanitized = true; } } } From 1e7a071e53bd8fe78e9755545ea5a9088ccd8ddf Mon Sep 17 00:00:00 2001 From: Scott Beddall Date: Fri, 26 Jul 2024 10:47:08 -0700 Subject: [PATCH 6/8] add entry remove, call it --- .../Common/RecordSession.cs | 12 ++++++++++-- .../Azure.Sdk.Tools.TestProxy/RecordingHandler.cs | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/RecordSession.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/RecordSession.cs index 88073ca39ae..a0414ef9441 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/RecordSession.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/RecordSession.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. using System; @@ -84,7 +84,7 @@ public void Record(RecordEntry entry) public RecordEntry Lookup(RecordEntry requestEntry, RecordMatcher matcher, IEnumerable sanitizers, bool remove = true) { - foreach(RecordedTestSanitizer sanitizer in sanitizers) + foreach (RecordedTestSanitizer sanitizer in sanitizers) { sanitizer.Sanitize(requestEntry); } @@ -103,6 +103,14 @@ public RecordEntry Lookup(RecordEntry requestEntry, RecordMatcher matcher, IEnum } } + public void Remove(RecordEntry entry) + { + lock (Entries) + { + Entries.Remove(entry); + } + } + public void Sanitize(RecordedTestSanitizer sanitizer) { lock (Entries) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs index 08511273dd8..7690acf82cf 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs @@ -521,7 +521,7 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming if (remove) { - session.Session.Lookup(entry, session.CustomMatcher ?? Matcher, sanitizers, remove: true); + session.Session.Remove(match); } } From c3af4b5c4e8ac03c7072d8da083c1ecbafe4ed39 Mon Sep 17 00:00:00 2001 From: Scott Beddall Date: Sun, 28 Jul 2024 19:34:16 -0700 Subject: [PATCH 7/8] more effective lock utilization --- .../Common/RecordSession.cs | 10 ++++++++++ .../Azure.Sdk.Tools.TestProxy/RecordingHandler.cs | 12 ++---------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/RecordSession.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/RecordSession.cs index a0414ef9441..8cd39ca831b 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/RecordSession.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/RecordSession.cs @@ -118,5 +118,15 @@ public void Sanitize(RecordedTestSanitizer sanitizer) sanitizer.Sanitize(this); } } + public void Sanitize(IEnumerable sanitizers) + { + lock (Entries) + { + foreach(var sanitizer in sanitizers) + { + sanitizer.Sanitize(this); + } + } + } } } diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs index 7690acf82cf..04e6f55bfb7 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs @@ -114,11 +114,7 @@ public void StopRecording(string sessionId, IDictionary variable } var sanitizers = SanitizerRegistry.GetSanitizers(recordingSession); - - foreach (RecordedTestSanitizer sanitizer in sanitizers) - { - recordingSession.Session.Sanitize(sanitizer); - } + recordingSession.Session.Sanitize(sanitizers); if (variables != null) { @@ -464,11 +460,7 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming { if (!session.IsSanitized) { - foreach (RecordedTestSanitizer sanitizer in sanitizers) - { - session.Session.Sanitize(sanitizer); - } - + session.Session.Sanitize(sanitizers); session.IsSanitized = true; } } From c609006f32c335dd51941ea57cf663650825a575 Mon Sep 17 00:00:00 2001 From: Scott Beddall Date: Mon, 29 Jul 2024 13:48:00 -0700 Subject: [PATCH 8/8] I think this will resolve the wonky no record matching errors. has to --- tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs index 04e6f55bfb7..c0d892ef6f1 100644 --- a/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs +++ b/tools/test-proxy/Azure.Sdk.Tools.TestProxy/RecordingHandler.cs @@ -456,7 +456,7 @@ public async Task HandlePlaybackRequest(string recordingId, HttpRequest incoming if (!session.IsSanitized) { // we don't need to re-sanitize with recording-applicable sanitizers every time. just the very first one - lock (session) + lock (session.SanitizerLock) { if (!session.IsSanitized) {