Coding best practices: `async` vs `await` improvements #2156

henriqueholtz · 2024-10-01T11:36:13Z

Async/await improvements according to .NET AsyncGuidance

Using .GetAwaiter().GetResult() instead of .Result;
Getting rid some warnings;
Turning the tests as async/await as possible. Getting rid of:
- .Wait()
- .Result
- .GetAwaiter().GetResult()

Notes:

I know the PR became huge, but most changes are pretty similar thus easier to review.
Some tests from Ocelot.AcceptanceTests were already failing (once in a while) at develop branch even before my changes, specially:
- KubernetesServiceDiscoveryTests.ShouldHighlyLoadOnUnstableKubeProvider_WithRoundRobinLoadBalancing
- ConsulServiceDiscoveryTests.Should_send_request_to_service_after_it_becomes_available_in_consul

raman-m · 2024-10-04T15:58:34Z

Hello, Henrique! Welcome to Ocelot!

Indeed, we have an issue with the improper use of threading operators, a legacy problem from the early days of Ocelot.
We are addressing it gradually. Your PR, however, has impressed me.

Async/await improvements according to .NET AsyncAwaitBestPractices

Are you aware that Microsoft Learn also provides documentation on best practices? I will find some links for you later.
For the moment, you need to resolve all merge conflicts.
Have you rebased onto develop following the Oct 3 sync to main?

raman-m · 2024-10-04T16:01:02Z

Some tests from Ocelot.AcceptanceTests were already failing (once in a while) at develop branch even before my changes, specially:

KubernetesServiceDiscoveryTests.ShouldHighlyLoadOnUnstableKubeProvider_WithRoundRobinLoadBalancing

ConsulServiceDiscoveryTests.Should_send_request_to_service_after_it_becomes_available_in_consul

No worries, I will review them later.

henriqueholtz · 2024-10-05T09:42:39Z

{...} Your PR, however, has impressed me.

Really ? Thanks for the feedback. I wasn't sure if it would be useful, specially because the PR's size.

Are you aware that Microsoft Learn also provides documentation on best practices? I will find some links for you later. For the moment, you need to resolve all merge conflicts.

I'm not sure, I'll be thankful to get these links.

Have you rebased onto develop following the Oct 3 sync to main?

I only got based on develop branch. I'll resolve the conflicts and make sure the PR is up to date soon.

henriqueholtz · 2024-10-05T20:06:13Z

@raman-m as you can see I just solved the conflicts carefully.

The PR's check is failing at RunAcceptanceTests step, however this error only occurs eventually. Test which is intermittently failing (see the image below):

ConsulServiceDiscoveryTests.ShouldUseConsulServiceDiscoveryAndLoadBalanceRequestWhenDynamicRoutingWithNoRoutes

Note: Please let me know if you have any comments (good or not) about the changes, I'm really really interested on.

raman-m · 2024-10-08T15:59:32Z

Henrique, there's no need to be concerned about the failed tests. Indeed, we do have several unstable tests with a failure probability of 5-10% in CI-CD builds. They tend to be most unstable during US working hours when CircleCI experiences high traffic.
I've rerun the PR build, and it's passed now.
I am officially requesting a code review from the team...

raman-m

The primary issue with this PR appears to be the artificial changes resulting from an EOL discrepancy. For more details, please refer to the initial code review comment.

raman-m · 2024-10-08T16:43:19Z

test/Ocelot.AcceptanceTests/Steps.cs

+        DownstreamHostAndPorts = new() { Localhost(port) },
+        DownstreamScheme = Uri.UriSchemeHttp,
+        UpstreamPathTemplate = "/",
+        UpstreamHttpMethod = new() { HttpMethods.Get },


I'm dissatisfied with these superficial changes; it appears to be a line ending issue.
Which IDE do you utilize for rebasing your feature branches?
It's our common practice not to alter end-of-line characters. Additionally, we employ Visual Studio's specific .editorconfig settings for EOL to circumvent these line ending issues. These settings are exclusive to Visual Studio, which is why we advise rebasing a feature branch onto develop solely using Visual Studio.
Have you overridden the EOL settings in .gitattributes?

I'm using Visual Studio Community 2022, and I haven't overridden the .gitattributes neither .editorconfig. Should I install a specific extension?

I'm really interested on be within the rules of the project, but I'm not sure what's lacking (BTW, thanks for letting me know you're dissatisfied with that, I would be the same way).

It is hard to read PR changes having fake changes!
It's preferable to resolve merge conflicts while honoring the changes in the develop branch. It appears that changes are being collected from the feature branch, even when there are no substantial changes. However, conflicts should be resolved by applying your changes onto the develop branch using a merging tool.

If changes from the feature branch are prioritized (despite being insignificant), the merge tool will record them and apply CRLF end-of-line characters based on the rules specified in .editorconfig. This is where the issue arises.

test/Ocelot.AcceptanceTests/Steps.cs

raman-m · 2024-10-08T17:28:55Z

test/Ocelot.AcceptanceTests/Steps.cs

+    public async Task<HttpResponseMessage> WhenIGetUrlOnTheApiGatewayAsync(string url, string cookie, string value)
+    {
+        var header = new CookieHeaderValue(cookie, value);
+        return await WhenIGetUrlOnTheApiGatewayAsync(url, header);


Why do you await here? 🙄 You can simply return a Task!

I realize I misunderstand this part at the async guide at Prefer async/await over directly returning Task. I agree we should just return the Task.

Reading the piece below... Is it looks confuse for you too (specially the final paragraph)?

I can't dispute Mr. Fowler's point, and his advice is indeed beneficial for the library code, not the testing project. Performance is my priority during testing: tests need to be quick. I'm not concerned about leaking internal variables into the broader context in testing projects or a disrupted call stack.

Please fix this!

Already did!

test/Ocelot.AcceptanceTests/Steps.cs

test/Ocelot.Benchmarks/AllTheThingsBenchmarks.cs

test/Ocelot.IntegrationTests/CacheManagerTests.cs

raman-m · 2024-10-10T13:10:24Z

Is the PR prepared for the second round of code review?

Revert all renaming in testing projects!

Converting synchronous methods to asynchronous should be done discreetly, without renaming the methods!
In testing projects, we have exceptions and overlook certain coding conventions. Our focus is on the tests, not the coding conventions. For these projects, we utilize StyleCop, which offers adequate style checking.

P.S.

When you rename a method in Visual Studio or use another refactoring command, Visual Studio applies the command using the default styling rules in .editorconfig, which includes CRLF settings. Therefore, applying auto-refactoring commands implicitly changes the EOL characters! This is the root cause of fake changes in PRs.
To maintain the original EOL characters, you must edit the code manually!

raman-m · 2024-10-10T13:53:55Z

@RaynaldM I believe this PR is not ready to be merged!

raman-m · 2024-10-11T09:34:01Z

Hi, what have you decided about this PR? Will you keep this PR open or create a new one?

henriqueholtz · 2024-10-11T10:39:19Z

Hi, what have you decided about this PR? Will you keep this PR open or create a new one?

As I turned as draft and I'm pushing new commits, I'll try to keep this one. I'll mark as ready and request a new review later.

henriqueholtz · 2024-10-13T09:57:22Z

@raman-m I carefully improved the PR removing so many fake changes. There were 92 changed files before and now we have 60. Also into there 60 they have lesser changes in each (because I reverted the renaming adding the suffix Async).

Reverting fake changes was successfully done for most files using commands like below (origin in my case is pointing to original repository):

git checkout origin/develop -- test/Ocelot.AcceptanceTests/ButterflyTracingTests.cs

However I couldn't be able to get rid some fake changes at some files which has those fake changes but also some real changes like test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCacheTests.cs:

I also tried to revert and manually re-add the real changes in VS but VS Code and didn't work which looks to be a problem with some repository's config (probably at .editorconfig). I tried add some like eol=lf but didn't work as well.

Example:

Note: I could got rid the above fake changes doing: reverting all the changes using the command above and bringing changes manually using notepad++ . I know it's a bad idea but I did want to be sure if there is a problem only using the IDEs. You can see the attempts at PR's commit history.

I turned the PR as ready to review again with a few files at this scenario. If you want to try something feel free to ask or even to do directly (and let me know please).

Files which has some few fake changes yet:

test/Ocelot.UnitTests/Configuration/FileConfigurationSetterTests.cs
test/Ocelot.UnitTests/Requester/HttpRequesterMiddlewareTests.cs
test/Ocelot.UnitTests/Security/SecurityMiddlewareTests.cs
test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs
test/Ocelot.IntegrationTests/HeaderTests.cs
test/Ocelot.IntegrationTests/CacheManagerTests.cs
test/Ocelot.AcceptanceTests/Steps.cs
test/Ocelot.AcceptanceTests/AggregateTests.cs

raman-m · 2024-10-13T14:01:46Z

Henrique, you made a mistake before the creation of this PR →

feature branch: henriqueholtz:develop

The best practice is to always create a separate feature branch from develop, since develop is the default branch of the fork that needs to be synchronized with the upstream repository.

Additional commits in the default 'develop' branch can restrict actions such as rebasing and creating new branches. Rebasing, in particular, becomes impossible, leaving only manual work as a viable option. 🤔

raman-m

I'm uncertain who will correct the fictitious changes. Such tasks will consume our precious time...

src/Ocelot.Provider.Consul/Consul.cs

raman-m · 2024-10-13T14:29:22Z

src/Ocelot.Provider.Consul/PollConsul.cs

@@ -51,7 +51,7 @@ public Task<List<Service>> GetAsync()
            try
            {
                _logger.LogInformation(() => $"Retrieving new client information for service: {ServiceName}...");
-                _services = _consulServiceDiscoveryProvider.GetAsync().Result;
+                _services = _consulServiceDiscoveryProvider.GetAsync().GetAwaiter().GetResult();


Since we are within a lock statement, we must adhere to writing only synchronous code. I agree with the change, however...
@ggnaegi, what would occur if we eliminate the lock? Due to the need for precise time capturing, we must prevent other threads from executing because the method modifies the class member _lastUpdateTime on line 59. It appears it's not feasible to convert this method into truly asynchronous, correct?

Not an issue!.. We will tackle the issue of lock statements versus synchronous task calls in the future.

src/Ocelot/Configuration/Repository/ConsulFileConfigurationPollerOption.cs

src/Ocelot/Request/Mapper/StreamHttpContent.cs

raman-m · 2024-10-13T15:12:56Z

test/Ocelot.AcceptanceTests/AggregateTests.cs

Oh, my gosh! 🤯 👇 700 lines of fake changes!
Can't you make manual changes?

Advice: Copy a clean file from the develop branch in the upstream repository (which requires another copy of the upstream repo), and then apply your 2-3 line change. This is a straightforward manual modification, but it is always effective.

@raman-m I was trying to figure out why those "fake changes" even though we have the .editorconfig in place.

Note: the right version is CRLF (as it's in .editorconfig file).

Those "fake changes" are because some lines at these files are still as LF (incorrect!). It means that these changes are actually the correct way to do (I know it let the PR longer/harder to review).

Are you aware of that? Should I undo it even though ? It's important to point out that any person who touch these files using an IDE like VS or VS Code will hit the same problem.

Basically if I undo these "fake changes", when someone else touch these files they'll face the same issue. Considering the long way to the repo please consider that we should keep the changes as the .editorconfig say. Take a look at the example below.

Also you can just try to do that on your machine and see/confirm it.

Example test/Ocelot.UnitTests/Configuration/FileConfigurationSetterTests.cs:

I was trying to figure out why those "fake changes" even though we have the .editorconfig in place.

I've detailed the root cause of fictitious ("fake") changes here: they are the result of auto-refactoring commands in IDEs such as Visual Studio, Visual Code, Rider, and others.

Those "fake changes" are because some lines at these files are still as LF (incorrect!). It means that these changes are actually the correct way to do (I know it let the PR longer/harder to review).

This issue has persisted since the project's inception in 2016!
Indeed, some lines end with the LF character from the Linux OS. Several of our contributors work on Linux and use IDEs like Visual Code, where the default newline character is LF. Consequently, we have numerous files with inconsistent EOL characters. Do you code on Linux or Windows?
I suggest using Windows to avoid the end-of-line (EOL) issue. ❗

Are you aware of that? Should I undo it even though ? It's important to point out that any person who touch these files using an IDE like VS or VS Code will hit the same problem.

Yes, I am! What are you trying to teach me, man? I've been aware of this issue for over a year.
You'll encounter the problem if you use Linux OS.
Boot into Windows, use Visual Studio Community (it's free), avoid using auto-refactoring commands, and everything will work out fine.

Basically if I undo these "fake changes", when someone else touch these files they'll face the same issue. Considering the long way to the repo please consider that we should keep the changes as the .editorconfig say. Take a look at the example below.

It's irrelevant! The likelihood of encountering this issue remains unchanged. We cannot foresee which operating system the subsequent developer might use. The concern is not regarding future developers, but rather about your current development environment.
If your PR contains numerous artificial changes aka "fake changes", it increases the chances that subsequent developers will encounter this issue. Therefore, I request the removal of all such changes due to end-of-line characters.

Fixed in commit a851098 ✔️

Got the same issue using Windows (VS and VS Code), but also using linux. That's because I asked you gently, while I was trying to help. You was totally rough with no reason to act like that.

raman-m · 2024-10-16T08:36:10Z

@henriqueholtz I'm initiating a rebase of develop onto upstream develop...
Please refrain from pushing/pulling during this process❗ The rebase will address any inconsistencies in the branch, ensuring that all new commits are placed on top of the upstream develop commits.

…ating the param name to get rid the warning at "HttpHandlerOptionsCreator" (because its implementation call it as "options", and I kept as it's in the implementation because make more sense)

…cheTests => Reverting fake changes (using `git add --renormalize {fileName}`

…reRealCacheTests => Reverting fake changes (using `git add --renormalize {fileName}`" This reverts commit 4c6d1e7.

…eRealCacheTests => Turning method "WhenICallTheMiddleware" as async" This reverts commit 8d7743d.

…heTests => Turning method "WhenICallTheMiddleware" as async (using notepad++)

…s Task.Run Ref: #2156 (comment)

… Reverting changes

…Tests => Reverting changes" This reverts commit 07903f3.

raman-m

Going to fix "fake changes"...

src/Ocelot.Provider.Consul/Consul.cs

raman-m · 2024-10-16T09:44:51Z

src/Ocelot.Provider.Consul/PollConsul.cs

@@ -51,7 +51,7 @@ public Task<List<Service>> GetAsync()
            try
            {
                _logger.LogInformation(() => $"Retrieving new client information for service: {ServiceName}...");
-                _services = _consulServiceDiscoveryProvider.GetAsync().Result;
+                _services = _consulServiceDiscoveryProvider.GetAsync().GetAwaiter().GetResult();


Not an issue!.. We will tackle the issue of lock statements versus synchronous task calls in the future.

raman-m · 2024-10-16T09:51:00Z

src/Ocelot/Configuration/Repository/ConsulFileConfigurationPollerOption.cs

@@ -18,7 +18,7 @@ private int GetDelay()
        {
            var delay = 1000;

-            var fileConfig = Task.Run(async () => await _fileConfigurationRepository.Get()).Result;
+            var fileConfig = _fileConfigurationRepository.Get().GetAwaiter().GetResult();


We have this sync version in the interface:

Ocelot/src/Ocelot/Configuration/Repository/IFileConfigurationPollerOptions.cs

Line 5 in 0379c77

int Delay { get; }

Why not to add async version to the interface?
After that we can make private and public methods as true async → Task<int> DelayAsync { get; }

Suggested change

var fileConfig = _fileConfigurationRepository.Get().GetAwaiter().GetResult();

var fileConfig = await _fileConfigurationRepository.Get();

Optional goal is using Task.Delay, if applicable.

The expression has been revised to execute synchronously within the synchronous int GetDelay() method.
The proposed suggestion for refactoring is beyond the scope of this pull request. We will refactor the interface in future.
TODO added in commit 6ff1011 ✔️

src/Ocelot/Request/Mapper/StreamHttpContent.cs

…FluentValidatorTests.cs

raman-m

Ready for Final Code Review❕

Henrique, the "fake changes," also known as EOL implicit changes, have been removed.
Please halt all development for the following reasons:

Further development and optimization of testing projects are nonsensical; we have done sufficient work.
I will address the remaining issues in the main library.
I am not interested in continuing the development and enhancement of this PR.

It appears that reading Result once is acceptable when the value task is complete, as indicated by the ValueTask{TResult}.Result Property Remarks. Therefore, this change may be unnecessary due to the assurance provided by .IsCompletedSuccessfully. See remarks: https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.valuetask-1.result?view=net-8.0#remarks

Add comment with ToDo

raman-m

Ready for delivery ✅

raman-m · 2024-10-16T19:20:03Z

@henriqueholtz
Following the enhancements in the Steps class, our acceptance tests are now better equipped for increased parallel work. We will soon transition our acceptance tests to genuine concurrent ones by designating the project for parallel execution. I am optimistic that the new version of Steps will aid in their parallelization and stabilization.

Thank you for your contribution! ❤️
We eagerly anticipate more PRs from you. 😉

raman-m · 2024-10-16T19:23:40Z

P.S.

Please re-fork the Ocelot repository into your account due to the previously mentioned issue of having extra commits in the develop branch, which should only contain commits from the upstream develop.

You will need to re-fork the repository.

henriqueholtz marked this pull request as ready for review October 2, 2024 23:46

raman-m added the conflicts Feature branch has merge conflicts label Oct 4, 2024

raman-m assigned henriqueholtz Oct 4, 2024

raman-m added proposal Proposal for a new functionality in Ocelot Core Ocelot Core related or system upgrade (not a public feature) and removed conflicts Feature branch has merge conflicts labels Oct 8, 2024

raman-m requested review from RaynaldM, raman-m and ggnaegi October 8, 2024 15:59

raman-m requested changes Oct 8, 2024

View reviewed changes

RaynaldM approved these changes Oct 10, 2024

View reviewed changes

raman-m requested a review from RaynaldM October 10, 2024 13:51

henriqueholtz marked this pull request as draft October 10, 2024 14:46

henriqueholtz marked this pull request as ready for review October 13, 2024 09:58

henriqueholtz requested a review from raman-m October 13, 2024 09:58

raman-m requested changes Oct 13, 2024

View reviewed changes

henriqueholtz marked this pull request as draft October 13, 2024 18:52

chore: Ocelot.Configuration.Creator.IHttpHandlerOptionsCreator => Upd…

123c6e8

…ating the param name to get rid the warning at "HttpHandlerOptionsCreator" (because its implementation call it as "options", and I kept as it's in the implementation because make more sense)

henriqueholtz and others added 8 commits October 16, 2024 12:21

chore: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCa…

b6c3d82

…cheTests => Reverting fake changes (using `git add --renormalize {fileName}`

Revert "chore: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewa…

a38f733

…reRealCacheTests => Reverting fake changes (using `git add --renormalize {fileName}`" This reverts commit 4c6d1e7.

Revert "test: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewar…

0c1b38b

…eRealCacheTests => Turning method "WhenICallTheMiddleware" as async" This reverts commit 8d7743d.

test: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCac…

6896aaf

…heTests => Turning method "WhenICallTheMiddleware" as async (using notepad++)

perf: ConsulFileConfigurationPollerOption.GetDelay => Removing useles…

a9659ed

…s Task.Run Ref: #2156 (comment)

chore: Ocelot.UnitTests.Configuration.FileConfigurationSetterTests =>…

480ba72

… Reverting changes

Revert "chore: Ocelot.UnitTests.Configuration.FileConfigurationSetter…

3655677

…Tests => Reverting changes" This reverts commit 07903f3.

Fix build errors

b11b8ee

raman-m marked this pull request as ready for review October 16, 2024 09:39

raman-m requested changes Oct 16, 2024

View reviewed changes

raman-m added 9 commits October 16, 2024 13:02

EOL: test/Ocelot.AcceptanceTests/AggregateTests.cs

a851098

code review by @raman-m

5ea8c89

EOL: test/Ocelot.AcceptanceTests/Steps.cs

efd5a31

Fix build

16e495f

EOL: test/Ocelot.IntegrationTests/CacheManagerTests.cs

8061814

EOL: test/Ocelot.IntegrationTests/HeaderTests.cs

5c03c96

EOL: test/Ocelot.UnitTests/Configuration/Validation/FileConfiguration…

943d9bb

…FluentValidatorTests.cs

EOL: test/Ocelot.UnitTests/Requester/HttpRequesterMiddlewareTests.cs

93331c7

EOL: test/Ocelot.UnitTests/Security/SecurityMiddlewareTests.cs

cc4a920

raman-m reviewed Oct 16, 2024

View reviewed changes

raman-m added 2 commits October 16, 2024 17:58

Sync call of the async method inside of sync context.

6ff1011

Add comment with ToDo

raman-m approved these changes Oct 16, 2024

View reviewed changes

raman-m added the Oct'24 October 2024 release label Oct 16, 2024

raman-m added this to the October'24 milestone Oct 16, 2024

Add docs for Dev Best Practices

5376507

raman-m changed the title ~~Async/await improvements~~ Coding best practices: async vs await improvements Oct 16, 2024

raman-m merged commit ce0227c into ThreeMammals:develop Oct 16, 2024
1 check passed

	var fileConfig = _fileConfigurationRepository.Get().GetAwaiter().GetResult();
	var fileConfig = await _fileConfigurationRepository.Get();

Coding best practices: async vs await improvements #2156

Coding best practices: async vs await improvements #2156

Conversation

henriqueholtz commented Oct 1, 2024 • edited Loading

raman-m commented Oct 4, 2024

raman-m commented Oct 4, 2024

henriqueholtz commented Oct 5, 2024

henriqueholtz commented Oct 5, 2024

raman-m commented Oct 8, 2024

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

henriqueholtz Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m commented Oct 10, 2024 • edited Loading

Is the PR prepared for the second round of code review?

Revert all renaming in testing projects!

P.S.

raman-m commented Oct 10, 2024

raman-m commented Oct 11, 2024

henriqueholtz commented Oct 11, 2024

henriqueholtz commented Oct 13, 2024

raman-m commented Oct 13, 2024

raman-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m commented Oct 16, 2024

raman-m left a comment

Choose a reason for hiding this comment

raman-m Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m left a comment

Choose a reason for hiding this comment

Ready for Final Code Review❕

raman-m left a comment

Choose a reason for hiding this comment

Ready for delivery ✅

raman-m commented Oct 16, 2024

raman-m commented Oct 16, 2024

P.S.

Coding best practices: `async` vs `await` improvements #2156

Coding best practices: `async` vs `await` improvements #2156

henriqueholtz commented Oct 1, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m Oct 8, 2024 •

edited

Loading

henriqueholtz Oct 10, 2024 •

edited

Loading

raman-m commented Oct 10, 2024 •

edited

Loading

raman-m Oct 16, 2024 •

edited

Loading

raman-m Oct 13, 2024 •

edited

Loading

raman-m Oct 16, 2024 •

edited

Loading

raman-m Oct 16, 2024 •

edited

Loading

raman-m Oct 16, 2024 •

edited

Loading