refactor: make the C# API properly async end-to-end - #22
Draft
hahn-kev wants to merge 6 commits into
Draft
Conversation
Convert the request-handling chain from synchronous/blocking to async all the way down, removing thread-pool starvation on the hot paths. - ProcessRunner.RunSync -> RunAsync (WaitForExitAsync; drop sync-over-async) - AsyncRunner: WaitForIsCompleteAsync (Task.Delay), async lock-file I/O; background-task design retained (its task stays on CancellationToken.None so it survives the originating request) - HgRunner / HgResumeApi: async Task methods throughout; the up-to-14s pull Thread.Sleep loop and 5s validate loop become await Task.Delay; async bundle-file streaming (GetChunkAsync, async append write) - RestDispatcher: DispatchAsync; HttpContext.RequestAborted threaded end to end; OperationCanceledException on disconnect no longer mapped to a bogus FAIL/RESET Behavior-preserving: the X-HgR-* wire contract, explicit Content-Length, status codes, and poll timing (5x1s / 7x2s) are unchanged. Tiny metadata JSON I/O intentionally left synchronous. All 46 HttpTests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collapse the two-job Docker workflow into a single build -> test -> push job following Docker's test-before-push pattern: build+load the amd64 image, run the tests against it, then build+push the multi-arch image reusing the amd64 layers from the builder cache (only arm64 is built for the push). Login/push gating for fork PRs is unchanged. Action versions bumped to latest majors. Also run HgResume.SendReceiveTests (real Chorus resumable client) in CI, not just HttpTests. They were never OS-guarded in code; the only blocker was the csproj force-copying a checked-in Windows hg.exe bundle on every platform. Stage that bundle on Windows only so on Linux the cross-platform hg from SIL.Chorus.Mercurial is used instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A job-wide VERSION env var is imported by MSBuild as the $(Version) property, which made `dotnet test` fail with "'v<date>' is not a valid version string" once build/test/push shared one job. Emit the date tag as a step output and reference it from metadata-action instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Only the Windows hg.exe bundle (Mercurial\) should be Windows-only. The MercurialExtensions\fixutf8 directory is cross-platform Python that Chorus requires on every OS (HgRepository.CheckMercurialIni), so gating it broke the Linux send/receive run with "Could not find the directory MercurialExtensions/fixutf8". Stage it unconditionally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MSBuild on Linux does not normalize backslashes in Include globs, so "MercurialExtensions\**" matched nothing on the runner and Chorus couldn't find MercurialExtensions/fixutf8. Forward-slash globs match on both Windows and Linux. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The <Content Include> glob for the package-provided Mercurial/ and MercurialExtensions/ dirs is evaluated before SIL.Chorus.Mercurial's CopyFiles target drops them into the project dir, so on a clean build nothing reached the output dir and Chorus failed with "Could not find MercurialExtensions/ fixutf8". Replace the includes with an AfterTargets=Build target whose globs run after CopyFiles. MercurialExtensions (from runtimes/any) is staged on all platforms; the win hg (Mercurial/) only on Windows, since on Linux NuGet already copies the linux-x64 hg to output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
This is a beautiful example of using AI to squash some tech debt - well done! |
Collaborator
Author
|
Yeah it's pretty sweet. I'm hoping next to point a profiler at the full stack and see if we can make it super fast. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should help us to avoid running out of threads, it also gives us cooperative cancellation, this should keep us from running work when the client aborts or cancels it's request.
🤖 AI summary
The C# API was a faithful, behavior-preserving rewrite of the PHP app in which only the HTTP entry layer (
RestDispatcher) was async — everything below it ran synchronously and blocked request threads. Most notably, twoThread.Sleeppoll loops sat on the request thread (up to 14s on the pull path, 5s on validate/unbundle), plus sync-over-async process execution and synchronous large-file streaming. This converts the whole request-handling chain to be async from top to bottom.Changes (bottom-up):
ProcessRunner—RunSync→RunAsyncusingWaitForExitAsync; removed the.GetAwaiter().GetResult()sync-over-async.AsyncRunner—WaitForIsComplete→WaitForIsCompleteAsync(Thread.Sleep→Task.Delay); async lock-file read/writes. The static-registry background-task design is intentionally kept (it already survives the originating request); its background write deliberately stays onCancellationToken.None.HgRunner— revision/branch/validate/bundle methods are nowasync Taskwith aCancellationToken; pure launch methods stayed synchronous.HgResumeApi— all endpoints areasync Task<HgResumeResponse>; the 7×2s pullThread.Sleeploop and the validate loop becomeawait Task.Delay; bundle-file streaming is async (GetChunkAsync+ async append-write).RestDispatcher—Dispatch→DispatchAsync;HttpContext.RequestAbortedis threaded end-to-end through body read, dispatch, and response write.Best-practice details:
OperationCanceledExceptionfrom a client disconnect is caught-and-rethrown before the broadcatchblocks so a disconnect can't be mapped to a bogusFAIL/RESETor corrupt the resumable transaction state. AsyncFileStreams useFileOptions.Asynchronous. NoConfigureAwait(false)was sprinkled into app code (unnecessary in ASP.NET Core). Per scope, the tiny (<1KB) per-transaction metadata JSON I/O was intentionally left synchronous, and no DI was introduced.Behavior is preserved: the
X-HgR-*wire contract, explicitContent-Length, status-code mapping, and the client-visible poll timing (5×1s / 7×2s) are all unchanged — only the blocking mechanism differs.Test plan
dotnet build HgResume.slnx— clean, 0 warnings / 0 errors.Thread.Sleep, syncWaitForExit(), or.GetAwaiter().GetResult()remain insrc/(the residual.Resultreads are post-await Task.WhenAll, i.e. non-blocking).HgResume.HttpTestspass against the podman container (push/pull resume flows,InProgresspolling via the now-Task.Delayloop, unrelated-repo detection, offset/SOW handling, and themanyRevsHgRepoIsValidBasepagination cases).This change is