Skip to content
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

Jump to bottom

HTTP stress test improvements #42313

Merged
merged 8 commits into from
Sep 23, 2020
Merged

HTTP stress test improvements #42313

merged 8 commits into from
Sep 23, 2020

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Sep 16, 2020 •
edited
Loading

Fixes:

  • stress client double read of content fixed
  • fixed stress client hangs at start and stop
  • leveraged HttpVersionPolicy
  • increased pipeline timeout since we doubled the runs
  • fixed base docker images to avoid missing IO.Pipelines Kestrel exception.

Re-hauled tracing:

  • added server file logging
  • added log file rotation

Minor renames.

Contributes to: #42211 and #42198

@ghost
Copy link

ghost commented Sep 16, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP changed the title [WIP] HTTP stress test improvments HTTP stress test improvments Sep 17, 2020
@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP changed the title HTTP stress test improvments HTTP stress test improvements Sep 17, 2020
@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP marked this pull request as ready for review September 18, 2020 12:47
@ManickaP ManickaP requested a review from a team September 18, 2020 12:47
@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov
antonfirsov approved these changes Sep 21, 2020
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Looks good, and I can confirm this fixed the System.IO.Pipelines issue.
But probably still needs an other pair of eyes, more experienced with the HttpStress code.

src/libraries/System.Net.Http/tests/StressTests/HttpStress/HttpEventListener.cs Outdated
@@ -25,63 +44,63 @@ protected override void OnEventSourceCreated(EventSource eventSource)
}
}

protected override void OnEventWritten(EventWrittenEventArgs eventData)
private async Task ProcessMessages()
Copy link
Member

Choose a reason for hiding this comment

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

NIT: ProcessMessagesAsync()

src/libraries/System.Net.Http/tests/StressTests/HttpStress/Configuration.cs
@@ -43,8 +43,8 @@ public class Configuration
public double CancellationProbability { get; set; }

public bool UseHttpSys { get; set; }
public string? LogPath { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

So there's no value having this configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

My goal was to make the logging usable for my investigations (file rotation, client and server side logging). To make it work with customizable path seemed like too much effort with a little gain.

src/libraries/System.Net.Http/tests/StressTests/HttpStress/HttpStress.csproj
Comment on lines +12 to +13
<PackageReference Include="Serilog.Extensions.Logging.File" Version="2.0.0" />
<PackageReference Include="Serilog.Sinks.File" Version="4.1.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any rules about using 3rd-party helpers of this kind?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. I didn't even considered it here since it's completely separate tool from production code.

src/libraries/System.Net.Http/tests/StressTests/HttpStress/StressClient.cs
@@ -19,8 +19,6 @@ namespace HttpStress
{
public class StressClient : IDisposable
{
private const string UNENCRYPTED_HTTP2_ENV_VAR = "DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_HTTP2UNENCRYPTEDSUPPORT";
Copy link
Member

Choose a reason for hiding this comment

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

This is being removed because #987 got implemented right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, this setting doesn't exist anymore and was replaced by usage of HttpVersionPolicy here: https:/dotnet/runtime/pull/42313/files#diff-6e9155f2664d22f1f4e0e8288b9c5719R73

@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member Author

/azp run runtime-libraries stress-ssl

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP merged commit 1edad6a into dotnet:master Sep 23, 2020
@ManickaP ManickaP deleted the mapichov/http_stress_fixes branch September 23, 2020 15:41
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@antonfirsov antonfirsov antonfirsov approved these changes

Assignees
No one assigned
Labels
area-System.Net.Http
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ManickaP @antonfirsov @karelz @Dotnet-GitSync-Bot