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

Fix Easy Racer .NET tests (fix async/await) #223

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

HofmeisterAn
Copy link
Contributor

Hi 👋, I came across this project and noticed some issues/flaws in the .NET implementation. After investigating, I propose the following changes:

  1. The usage of async void is generally not recommended (except for events). I couldn't find a reason why the tests should use this method signature; using async Task is the preferred approach and is recommended.
  2. Synchronously waiting on an asynchronous operation (sync over async) is also not recommended and should be avoided. It blocks (wastes) one thread, which can lead to thread starvation and deadlocks.
  3. It is recommended to let Testcontainers resolve the actual Hostname. Depending on the container runtime and environment, the host can differ. Using the static value localhost can cause further issues. There is a bug or an issue in Docker Desktop where the IPv4/IPv6 binding does not get the same port assigned. Depending on how the operating system resolves localhost, it is necessary to resolve to the correct randomly assigned host port (find further information here).
  4. Scenario 10 does not consider the culture when converting the double CPU usage to a string. This breaks the HTTP request because, depending on the user's OS and settings, 0.95d.ToString() will be 0,95 instead of 0.95.

Before the changes, I was not able to run the tests. With the mentioned changes, the tests now run fine. However, TestScenario3 is still sometimes flaky. Unfortunately, I haven't had the time to take a closer look at it.

@delormej
Copy link

@HofmeisterAn thanks for the feedback! I'll review this in depth and make the appropriate fixes, including digging into Scenario 3 again. Thanks again for looking!

@jamesward
Copy link
Owner

Thank you @HofmeisterAn for the improvements!

@HofmeisterAn
Copy link
Contributor Author

👍 If you need help with the changes or have questions, feel free to reach out to me. I am happy help. Perhaps I will find some time to look into TestScenario3 too.

Choose a reason for hiding this comment

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

Thanks for catching the hardcoded localhost hostname in the url here. Any reason why we should just the URL and port on the Library itself instead of passing it to each method call? I was following @jamesward pattern for the others, but seems like this should be captured at the library level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was considering something similar, but I did not want to introduce unnecessary changes. Setting the BaseAddress of HttpClient as follows:

using var httpClient = new HttpClient();
// TODO: Set it to the correct container address.
httpClient.BaseAddress = new Uri("http://localhost:8080");

// Calls http://localhost:8080/1
_ = await httpClient.GetAsync("1");

// Calls http://localhost:8080/2
_ = await httpClient.GetAsync("2");

makes the GetUrl(string, ushort) method obsolete.

return answer;
}

private async Task<string> Monitor(string url)
{
while (true)
{
await Task.Delay(TimeSpan.FromSeconds(1));
await Task.Delay(TimeSpan.FromSeconds(0.1));

Choose a reason for hiding this comment

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

The instructions (https://github.com/jamesward/easyracer) are looking for this to run every 1 second.

Part 2) In parallel to Part 1, every 1 second, make a request with the current process load (0 to 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch; I think this was by accident. I will change it 👍.

@jjdelorme
Copy link

jjdelorme commented Mar 13, 2024

Quick note on why Scenario 3 is breaking when running it multiple times... It turns out there is an InnerException buried in the stack which is System.Net.Sockets.SocketException (104): Connection reset by peer. Looks like it's some kind of denial of service detection; https://stackoverflow.com/questions/57813659/why-is-there-a-threshold-of-concurrent-http-requests-i-can-make-using-httpclien

I've fixed this by implementing a simple RetryHandler, but I'll make it a little more robust with exponential back-off.

@jamesward jamesward merged commit c132f45 into jamesward:main Apr 8, 2024
1 check 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.

4 participants