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.

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

feat: support SslProtocol parameter for httpClient job #786

Conversation

DeagleGross
Copy link
Contributor

@DeagleGross DeagleGross commented Jan 20, 2025

Adding an option to explicitly set the sslProtocol version to use on SocketsHttpHandler

long throughput = 0;
try
{
throughput = transferred / ((sw.ElapsedTicks - measuringStart) / Stopwatch.Frequency);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was running the following job (needs adjustment for running without local changes)

crank --config https://raw.githubusercontent.com/DeagleGross/Benchmarks/refs/heads/dmkorolev/idna/tls-protocols/scenarios/tls.benchmarks.yml --scenario tls-handshakes-httpsys --profile aspnet-gold-win --application.framework net10.0 --application.options.outputFiles "D:\code\Benchmarks\src\BenchmarksApps\TLS\HttpSys\bin\Release\net9.0*.dll" --application.variables.logRequestDetails "true" --load.variables.sslProtocols "tls13"

and it fails with such exception (below), therefore I decided to handle it. Not sure why that happens only for tls13 scenario.

[STDERR] Unhandled exception. System.DivideByZeroException: Attempted to divide by zero.
[STDERR]    at Microsoft.Crank.Jobs.HttpClientClient.Program.DoWorkAsync() in /tmp/benchmarks-agent/benchmarks-server-1/pzqitig2.gjs/src/Microsoft.Crank.Jobs.HttpClient/Program.cs:line 718
[STDERR]    at Microsoft.Crank.Jobs.HttpClientClient.Program.RunAsync() in /tmp/benchmarks-agent/benchmarks-server-1/pzqitig2.gjs/src/Microsoft.Crank.Jobs.HttpClient/Program.cs:line 388
[STDERR]    at Microsoft.Crank.Jobs.HttpClientClient.Program.<>c__DisplayClass86_0.<<Main>b__1>d.MoveNext() in /tmp/benchmarks-agent/benchmarks-server-1/pzqitig2.gjs/src/Microsoft.Crank.Jobs.HttpClient/Program.cs:line 295
[STDERR] --- End of stack trace from previous location ---
[STDERR]    at McMaster.Extensions.CommandLineUtils.CommandLineApplicationExtensions.<>c__DisplayClass8_0.<<OnExecuteAsync>b__0>d.MoveNext()
[STDERR] --- End of stack trace from previous location ---
[STDERR]    at McMaster.Extensions.CommandLineUtils.CommandLineApplication.ExecuteAsync(String[] args, CancellationToken cancellationToken)
[STDERR]    at Microsoft.Crank.Jobs.HttpClientClient.Program.Main(String[] args) in /tmp/benchmarks-agent/benchmarks-server-1/pzqitig2.gjs/src/Microsoft.Crank.Jobs.HttpClient/Program.cs:line 298
[STDERR]    at Microsoft.Crank.Jobs.HttpClientClient.Program.<Main>(String[] args)

Copy link
Member

Choose a reason for hiding this comment

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

DivideByZeroException is one of the exception types you generally shouldn't be explicitly catching. We should instead figure out why it happened and fix that, or change the logic so we avoid dividing by zero in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

sw.ElapsedTicks could be equal to measuringStart if:

  • they are both not changes (default is 0)
  • code is so fast or optimized that sw.ElapsedTicks is not updated after measuringStart is assigned

To solve this we could initialize measuringStart with -1 and also set measuringStart = sw.ElapsedTicks - 1 to ensure they are never the same. The computation would be off by 1 tick, which wouldn't matter. And if they were to be identical then we can't get anything from the result so the newly non-zero value is as meaningless. While no exception will be thrown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, I adjusted the code as proposed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the (double) cast to be 100% sure there is no division by zero

@DeagleGross DeagleGross merged commit 9cb3b26 into dotnet:main Jan 21, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants