Coding best practices: async vs await improvements (#2156)

* chore: Ocelot.Configuration.Creator.IHttpHandlerOptionsCreator => Updating 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)

* chore: Getting rid of ".Result" (using "GetAwaiter().GetResult()" instead)

* test: Getting rid of ".Result" (using "GetAwaiter().GetResult()" instead)

* chore: Benchmarks => Turning "await app.UseOcelot()" instead of of "app.UseOcelot().Wait()".

* test: Turning "await app.UseOcelot()" instead of of "app.UseOcelot().Wait()".

* test: Ocelot.IntegrationTests.AdministrationTests => Turning methods as async instead of using "GetAwaiter().GetResult()"

* perf: Ocelot.UnitTests => Getting rid of "GetAwaiter().GetResult" and using as much async/await as possible

* test: Ocelot.AcceptanceTests => turning methods as async as possible

* test: Fixing the test "should_return_response_200_with_simple_url_when_using_jsonserialized_cache" at "ConfigurationInConsulTests" + async/await improvements

* test: Fixing tests from "ConsulConfigurationInConsulTests" + async/await imrpovements

* test: Fixing some tests after merging

* test: Ocelot.AcceptanceTests => Using asyn/await instead of ".GetAwaiter().GetResult()"

#2156 (comment)

* test: Returning the task directly instead of awaiting for it when it's just returned

* test: Undo moving the function "Steps.ThenTheStatusCodeShouldBe(HttpStatusCode expected)"

* test: Removing with no usage function "Steps.GivenOcelotIsRunningWithServices "

* test: Creating method "UntilAsync" and using it to get rid some ".GetAwaiter().GetResult()"

* chore: Reverting renaming (removing the suffix "Async")

* chore: (2th) Reverting renaming (removing the suffix "Async")

* test: Bringing back the slash I removed with no reason

* chore: Trying to revert fake changes of "test/Ocelot.AcceptanceTests/ButterflyTracingTests.cs"

* chore: Undone fake changes

* chore: Undone useless renamed methods

* chore: removing unecessary "await task" and just returning it

* chore: Undone fake changes "AggregateTests"

* chore: FileConfigurationFluentValidatorTests => Undo rename method "WhenIValidateTheConfigurationAsync" back to "WhenIValidateTheConfiguration"

* chore: test/Ocelot.AcceptanceTests/AggregateTests => reverting fake changes but keeping real changes

* chore: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCacheTests => undo all changes

* test: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCacheTests => Turning method "WhenICallTheMiddleware" as async

* perf: src/Ocelot.Provider.Consul/Consul => Using async/await instead of "GetAwaiter().GetResult()"

* chore: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCacheTests => Reverting fake changes (using `git add --renormalize {fileName}`

* Revert "chore: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCacheTests => Reverting fake changes (using `git add --renormalize {fileName}`"

This reverts commit 4c6d1e7.

* Revert "test: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCacheTests => Turning method "WhenICallTheMiddleware" as async"

This reverts commit 8d7743d.

* test: test/Ocelot.UnitTests/CacheManager/OutputCacheMiddlewareRealCacheTests => Turning method "WhenICallTheMiddleware" as async (using notepad++)

* perf: ConsulFileConfigurationPollerOption.GetDelay => Removing useless Task.Run

Ref: #2156 (comment)

* chore: Ocelot.UnitTests.Configuration.FileConfigurationSetterTests => Reverting changes

* Revert "chore: Ocelot.UnitTests.Configuration.FileConfigurationSetterTests => Reverting changes"

This reverts commit 07903f3.

* Fix build errors

* EOL: test/Ocelot.AcceptanceTests/AggregateTests.cs

* code review by @raman-m

* EOL: test/Ocelot.AcceptanceTests/Steps.cs

* Fix build

* EOL: test/Ocelot.IntegrationTests/CacheManagerTests.cs

* EOL: test/Ocelot.IntegrationTests/HeaderTests.cs

* EOL: test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs

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

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

* Revert the code to read `ValueTask.Result` if completed.
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

* Sync call of the async method inside of sync context.
Add comment with ToDo

* Add docs for Dev Best Practices

---------

Co-authored-by: Raman Maksimchuk <[email protected]>

docs/building/devprocess.rst

@@ -0,0 +1,107 @@
+Development Process
+===================
+
+* The development process works best with `Gitflow <https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow>`_ branching.
+  Note, Ocelot team doesn't use `GitHub flow <https://docs.github.com/en/get-started/using-github/github-flow>`_ which is faster but not efficient in Ocelot delivery.
+* Contributors can do whatever they want on pull requests and feature branches to deliver a feature to **develop** branch.
+* Maintainers can do whatever they want on pull requests and merges to **main** will result in packages being released to GitHub and NuGet.
+* Finally, users follow :doc:`../building/devprocess`, but maintainers follow this :doc:`../building/releaseprocess`.
+
+Ocelot uses the following process to accept work into a merged commit in develop.
+
+
+1. User creates an issue or picks up an `existing issue <https://github.com/ThreeMammals/Ocelot/issues>`_ in GitHub.
+   An issue can be created by converting `discussion <https://github.com/ThreeMammals/Ocelot/discussions>`_ topics if necessary and agreed upon.
+
+2. User creates `a fork <https://docs.github.com/en/get-started/quickstart/fork-a-repo>`_ and branches from this
+   (unless a member of core team, they can just create a branch on the head repo) e.g. ``feature/xxx``, ``bug/xxx`` etc.
+   It doesn't really matter what the "xxx" is. It might make sense to use the issue number and maybe a short description. 
+
+3. When the contributor is happy with their work they can create a pull request against **develop** in GitHub with their changes.
+
+4. The Ocelot team will provide code review the PR and if all is good merge it, else they will suggest feedback that the user will need to act on.
+   In order to speed up getting a PR the contributor should think about the following:
+
+   - Have I covered all my changes with tests at unit and acceptance level?
+   - Have I updated any documentation that my changes may have affected?
+   - Does my feature make sense, have I checked all of Ocelot's other features to make sure it doesn't already exist?
+
+   In order for a PR to be merged the following must have occured:
+
+   - All new code is covered by unit tests.
+   - All new code has at least 1 acceptance test covering the happy path.
+   - Tests must have passed locally.
+   - Build must have green status.
+   - Build must not have slowed down dramatically.
+   - The main Ocelot package must not have taken on any non MS dependencies.
+
+6. After the PR is merged to **develop** the Ocelot NuGet packages will not be updated until a release is created!
+   The final step is to go back to GitHub and close any issues that are now fixed.
+   **Note**: All linked issues to the PR in **Development** settings (right side PR settings) will be closed automatically while merging the PR.
+   It is imperative that developer uses the "**Link an issue from this repository**" pop-up dialog of the **Development** settings!
+
+Notes
+-----
+
+All PR builds are done with CircleCI, see `Pipelines - ThreeMammals/Ocelot <https://circleci.com/gh/ThreeMammals/Ocelot/>`_.
+It is advisable to watch for build status, and if it is failed, trigger new build or ask online maintainers or code reviewers to make sure the current PR build is green.
+
+If anything is unclear or you get stuck in the process, please contact the `Ocelot Core Team <https://github.com/orgs/ThreeMammals/teams/ocelot-core>`_ members or repository maintainers.
+
+.. _dev-best-practices:
+
+Best Practices
+--------------
+
+* Ask for code review after Dev Complete stage, and resolve all issues in a provided feedback. Code is complete when solid code, appropriate unit and acceptance tests and docs update are written. 
+* Organize your development environment in Windows OS utilizing Visual Studio IDE. You can develop in Linux with other IDEs, but we don't recommend that. See more details in :ref:`dev-fun` subsection.
+* Ensure you are always online after creation of the PR/issue, so maintainers will contact you as fastest as they can.
+  Note, if you will be offline for a days, weeks, months, then maintainers have a right to put your work in low priority.
+  Your intention to contribute should be high which means to be always online and proactive.
+
+.. _dev-fun:
+
+Dev Fun
+--------
+
+This is a part of :ref:`dev-best-practices` but it is more funny D)
+
+Line-ending gotchas aka EOL fun
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+    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/mixed EOL characters.
+
+This problem is related to well-known End-of-Line characters dillema in cross-OS development.
+For Windows OS the EOL char is ``CRLF`` but for Linux it is ``LF``.
+Modern IDEs and Git repos detect inconsistancy of mixed EOLs in source files following own strategies.
+But GitHub "Files Changed" tool unfortunately detects a line change in these 2 scenarios: ``CRLF`` to ``LF`` and ``LF`` to ``CRLF`` changes, even there was no actual code change!
+Such a pull requests with fictitious ("fake") changes are always hard to review because the focus of the reviwer should be paid to actual code changes.
+
+    Please note, if the pull request is full of "fake" changes in **Files Changed** then code reviewer has a right not providing a code review marking PR as draft, or even closing it!
+
+It's our common practice not to alter end-of-line characters.
+Additionally, we employ Visual Studio's specific `.editorconfig <https://github.com/ThreeMammals/Ocelot/blob/develop/.editorconfig>`_ IDE analyzer 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.
+
+    Special EOL settings can be provided in ``.gitattributes`` file of the git repository. But we don't handle this currently.
+
+Our current recommendations for addressing the EOL issue are:
+
+* 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.
+
+* When you rename a method in IDE, for instance in Visual Studio, or use another auto-refactoring command, Visual Studio applies the command using the default styling rules in ``.editorconfig``,
+  which includes `CRLF settings <https://github.com/search?q=repo%3AThreeMammals%2FOcelot%20end_of_line&type=code>`_.
+  Therefore, applying auto-refactoring commands implicitly changes the EOL characters! This is the source of "fake" changes in PRs.
+  Please note, Visual Studio analyzers (IDE, StyleCop, etc.) recommends auto-refactoring too which could be applied implicitly.
+  To maintain the original EOL characters, you must edit the code manually!
+  So, fictitious ("fake") changes are the result of auto-refactoring commands in IDEs such as Visual Studio, Visual Code, Rider, and others.
+
+* **Our final recommendations: Boot into Windows, use Visual Studio Community (it's free), avoid using auto-refactoring commands, and EOLs should remain unchanged.**

docs/building/releaseprocess.rst

@@ -1,60 +1,51 @@
 Release Process
 ===============
 
-* The release process works best with `Gitflow <https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow>`_ branching. 
-* Contributors can do whatever they want on PRs and feature branches to deliver a feature to **develop** branch.
-* Maintainers can do whatever they want on PRs and merges to **main** will result in packages being released to GitHub and NuGet.
+* The release process works best with `Gitflow <https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow>`_ branching.
+  Note, Ocelot team doesn't use `GitHub flow <https://docs.github.com/en/get-started/using-github/github-flow>`_ which is faster but not efficient in Ocelot delivery.
+* Contributors can do whatever they want on pull requests and feature branches to deliver a feature to **develop** branch.
+* Maintainers can do whatever they want on pull requests and merges to **main** will result in packages being released to GitHub and NuGet.
+* Finally, users follow :doc:`../building/devprocess`, but maintainers follow this :doc:`../building/releaseprocess`.
 
 Ocelot uses the following process to accept work into the NuGet packages.
 
-1. User creates an issue or picks up an `existing issue <https://github.com/ThreeMammals/Ocelot/issues>`_ in GitHub.
-   An issue can be created by converting `discussion <https://github.com/ThreeMammals/Ocelot/discussions>`_ topics if necessary and agreed upon.
+1. Maintainers provide code review of pull request and if all is good merge it, else they will suggest feedback that the user will need to act on.
+   Extra help to contributors is welcomed via constant Pair Programming sessions: multiple code reviews, fixing code review issues, any problem solving.
 
-2. User creates `a fork <https://docs.github.com/en/get-started/quickstart/fork-a-repo>`_ and branches from this (unless a member of core team, they can just create a branch on the head repo) e.g. ``feature/xxx``, ``bug/xxx`` etc.
-   It doesn't really matter what the "xxx" is. It might make sense to use the issue number and maybe a short description. 
-
-3. When the contributor is happy with their work they can create a pull request against **develop** in GitHub with their changes.
-
-4. The maintainer must follow the `SemVer <https://semver.org/>`_ support for this is provided by `GitVersion <https://gitversion.net/docs/>`_.
+2. The maintainer must follow the `SemVer <https://semver.org/>`_ support for this is provided by `GitVersion <https://gitversion.net/docs/>`_.
    So if the maintainer needs to make breaking changes, be sure to use the correct commit message, so **GitVersion** uses the correct **SemVer** tags.
    Do not manually tag the Ocelot repo: this will break things!
 
-5. The Ocelot team will review the PR and if all is good merge it, else they will suggest feedback that the user will need to act on.
-
-   In order to speed up getting a PR the contributor should think about the following:
-
-   - Have I covered all my changes with tests at unit and acceptance level?
-   - Have I updated any documentation that my changes may have affected?
-   - Does my feature make sense, have I checked all of Ocelot's other features to make sure it doesn't already exist?
+3. After the PR is merged to **develop** the Ocelot NuGet packages will not be updated until a release is created.
+   And, when enough work has been completed to justify a new release, **develop** branch will be merged into **main** as ``release/X.Y.Z`` branch,
+   the release process will begin which builds the code, versions it, pushes artifacts to GitHub and NuGet packages to NuGet.
 
-   In order for a PR to be merged the following must have occured:
+4. Release engineer, the owner of integration tokens both on CircleCi and GitHub, automates each release build by the main building script aka ``build.cake``.
+   Release engineer is responsible for any DevOps at the organization, in any (sub)repositories, supporting the main building script.
 
-   - All new code is covered by unit tests.
-   - All new code has at least 1 acceptance test covering the happy path.
-   - Tests must have passed locally.
-   - Build must have green status.
-   - Build must not have slowed down dramatically.
-   - The main Ocelot package must not have taken on any non MS dependencies.
+5. Release engineer writes `ReleaseNotes.md <https://github.com/ThreeMammals/Ocelot/blob/main/README.md>`_ notifying community about
+   important artifacts of the release such as new/updated features, fixed bugs, updated documentation, breaking changes, contributors info, version upgrade instructions, etc.
 
-6. After the PR is merged to **develop** the Ocelot NuGet packages will not be updated until a release is created.
+6. The final step is to go back to GitHub and close current milestone ensuring the following:
 
-7. When enough work has been completed to justify a new release,
-   **develop** branch will be merged into **main** as **release/xxx** branch, the release process will begin which builds the code, versions it, pushes artifacts to GitHub and NuGet packages to NuGet.
+   * all issues in the milestone should be closed, the rest of work of open issues should be moved to the next milestone.
+   * all pull requests of the milestone should be closed, or moved to the next upcoming release milestone.
+   * Release Notes should be published to GitHub releases, with extra checking the text.
+   * Published release must be marked as the latest, if appropriate Nuget packages were successfully uploaded to `NuGet Gallery | ThreeMammals <https://www.nuget.org/profiles/ThreeMammals>`_ account.
 
-8. The final step is to go back to GitHub and close any issues that are now fixed.
-   **Note**: All linked issues to the PR in **Development** settings (right side PR settings) will be closed automatically while merging the PR.
-   It is imperative that developer uses the "**Link an issue from this repository**" pop-up dialog of the **Development** settings!
+7. Optional support of the major version ``2X.Y.0`` should be provided in such cases as Microsoft official patches, critical Ocelot defects of the major version.
+   Maintainers release patched versions ``2X.Y.xxx`` as hot-fixing patch-versions.
 
 Notes
 -----
 
 All NuGet package builds and releases are done with CircleCI, see `Pipelines - ThreeMammals/Ocelot <https://circleci.com/gh/ThreeMammals/Ocelot/>`_.
 
-Only Tom Pallister (owner) and Ocelot Core Team members (maintainers) can merge releases into **main** at the moment.
-This is to ensure there is a final `quality gate <#quality-gates>`_ in place. Tom is mainly looking for security issues on the final merge.
+Only `Tom Pallister <https://github.com/TomPallister>`_, `Raman Maksimchuk <https://github.com/raman-m>`_ (owners) and maintainers from `Ocelot Team <https://github.com/orgs/ThreeMammals/teams>`_ can merge releases into `main <https://github.com/ThreeMammals/Ocelot/tree/main>`_ at the moment.
+This is to ensure there is a final :ref:`quality-gates` in place.
+Maintainers are mainly looking for security issues on the final merge: see Step 7 in the process.
 
-We **do** follow this development and release process!
-If anything is unclear or you get stuck in the process, please contact the `Ocelot Core Team <https://github.com/orgs/ThreeMammals/teams/ocelot-core>`_ members or repository maintainers.
+.. _quality-gates:
 
 Quality Gates
 -------------

docs/index.rst

@@ -19,7 +19,7 @@ All **Features** are listed in alphabetical order.
 The primary features include :doc:`../features/configuration` and :doc:`../features/routing`.
 
 Additional tips for building Ocelot can be found in the **Building Ocelot** section.
-We adhere to a development process outlined in :doc:`../building/releaseprocess`.
+We adhere to a :doc:`../building/devprocess` outlined in :doc:`../building/releaseprocess`.
 
 .. admonition:: Table of Contents
 
@@ -76,5 +76,6 @@ We adhere to a development process outlined in :doc:`../building/releaseprocess`
     building/overview
     building/building
     building/tests
+    building/devprocess
     building/releaseprocess
 

docs/releasenotes.rst

@@ -179,4 +179,8 @@ Contributing
 
 For `ideas <https://github.com/ThreeMammals/Ocelot/discussions/categories/ideas>`_ and `questions <https://github.com/ThreeMammals/Ocelot/discussions/categories/q-a>`_, please post them in the `Ocelot Discussions <https://github.com/ThreeMammals/Ocelot/discussions>`_ space.
 
-Our development process is detailed in the :doc:`../building/releaseprocess` documentation.
+Our :doc:`../building/devprocess` is a part of successful :doc:`../building/releaseprocess`.
+If you are a new contributor, it is crucial to read :doc:`../building/devprocess` attentively to grasp our methods for efficient and swift feature delivery.
+We, as a team, advocate adhering to :ref:`dev-best-practices` throughout the development phase.
+
+We extend our best wishes for your successful contributions to the Ocelot product!

src/Ocelot.Provider.Consul/Consul.cs

@@ -30,9 +30,8 @@ public virtual async Task<List<Service>> GetAsync()
         var nodesTask = _consul.Catalog.Nodes();
 
         await Task.WhenAll(entriesTask, nodesTask);
-
-        var entries = entriesTask.Result.Response ?? Array.Empty<ServiceEntry>();
-        var nodes = nodesTask.Result.Response ?? Array.Empty<Node>();
+        var entries = (await entriesTask).Response ?? Array.Empty<ServiceEntry>();
+        var nodes = (await nodesTask).Response ?? Array.Empty<Node>();
         if (entries.Length == 0)
         {
             _logger.LogWarning(() => $"{nameof(Consul)} Provider: No service entries found for '{_configuration.KeyOfServiceInConsul}' service!");

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();
                 return Task.FromResult(_services);
             }
             finally

src/Ocelot/Configuration/Creator/IHttpHandlerOptionsCreator.cs

@@ -7,6 +7,6 @@ namespace Ocelot.Configuration.Creator
     /// </summary>
     public interface IHttpHandlerOptionsCreator
     {
-        HttpHandlerOptions Create(FileHttpHandlerOptions fileRoute);
+        HttpHandlerOptions Create(FileHttpHandlerOptions options);
     }
 }

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(); // sync call, so TODO extend IFileConfigurationPollerOptions interface with 2nd async method
             if (fileConfig?.Data?.GlobalConfiguration?.ServiceDiscoveryProvider != null &&
                     !fileConfig.IsError &&
                     fileConfig.Data.GlobalConfiguration.ServiceDiscoveryProvider.PollingInterval > 0)

src/Ocelot/Request/Mapper/StreamHttpContent.cs

@@ -56,7 +56,9 @@ private static async Task CopyAsync(Stream input, Stream output, long announcedC
                 if (zeroByteReadTask.IsCompletedSuccessfully)
                 {
                     // Consume the ValueTask's result in case it is backed by an IValueTaskSource
-                    _ = zeroByteReadTask.Result;
+                    // It is save to read the Result once after the ValueTask has completed, and we've checked for complition by IsCompletedSuccessfully property
+                    // See remarks: https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.valuetask-1.result?view=net-8.0#remarks
+                    _ = zeroByteReadTask.Result; // No need to await the task by .GetAwaiter().GetResult()
                 }
                 else
                 {

test/Ocelot.AcceptanceTests/AggregateTests.cs

@@ -746,7 +746,7 @@ private void GivenOcelotIsRunningWithSpecificAggregatorsRegisteredInDi<TAggregat
                     s.AddOcelot()
                         .AddSingletonDefinedAggregator<TAggregator>();
                 })
-                .Configure(a => { a.UseOcelot().Wait(); });
+                .Configure(async b => await b.UseOcelot());
 
             _ocelotServer = new TestServer(_webHostBuilder);
             _ocelotClient = _ocelotServer.CreateClient();