Add client-side async API for replication (IAsyncReplicator) - #1182
Conversation
paulirwin
left a comment
There was a problem hiding this comment.
Thanks for the PR! I have some changes that need to be made, and a larger one about a refactoring that is up for discussion. Please share your thoughts on the refactoring before embarking on it!
|
@paulirwin @NightOwl888 |
25f65ff to
8135524
Compare
|
@NightOwl888 Reason: The merge brought in changes from master that we do not want in this feature branch. All intended feature commits are still intact. This keeps the branch history clean and focused on the feature work. If any changes from master are important to include, please let me know, and we can merge them properly. |
|
I’m seeing the check-editorconfig CI fail due to a “final newline expected” error. Locally, git diff --check doesn’t show any issues, so I suspect it might be related to line endings (CRLF vs LF) or some subtle trailing whitespace. Could you please guide me on the safest way to fix this, such as how to identify files with extra trailing whitespace or missing newlines, so the CI passes without affecting other files? |
|
That error means that after all of the content in the file, there is no newline character. So, it just needs to be added. I know it is a bit confusing - remove all trailing whitespace except at the end of the file, add a line break. |
2773b42 to
c660b52
Compare
|
I have rebased this PR on latest master. Had to do #1221 to fix the build error due to an editorconfig-checker update. I'll look at resolving the remaining comments. |
152c397 to
a3287bf
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a Task-based async API for client-side replication to avoid blocking/synchronous HTTP calls, while keeping the existing synchronous replication flow intact.
Changes:
- Introduces
IAsyncReplicatorand async counterparts to core replication operations (check/update, obtain file, release). - Updates
HttpReplicator/HttpClientBasewith async HTTP helpers and stream-handling utilities. - Extends
ReplicationClientwithUpdateNowAsync()and a periodic async update loop, plus nullability annotations across replicator-related APIs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Lucene.Net/Util/IOUtils.cs | Adjusts nullability for rethrow helpers (and minor doc text). |
| src/Lucene.Net.Tests.Replicator/Http/HttpReplicatorTest.cs | Adds async tests for client update and server error scenarios. |
| src/Lucene.Net.Replicator/Replicator.cs | Updates IReplicator.CheckForUpdate nullability annotations. |
| src/Lucene.Net.Replicator/ReplicationClient.cs | Adds async update APIs/loop and refactors locking to support async execution. |
| src/Lucene.Net.Replicator/IndexReplicationHandler.cs | Applies nullable reference annotations and modernizes string formatting. |
| src/Lucene.Net.Replicator/IndexAndTaxonomyReplicationHandler.cs | Applies nullable reference annotations and improves error formatting/guards. |
| src/Lucene.Net.Replicator/IAsyncReplicator.cs | Adds the new async replicator interface contract. |
| src/Lucene.Net.Replicator/Http/HttpReplicator.cs | Implements IAsyncReplicator with async HTTP request/stream handling. |
| src/Lucene.Net.Replicator/Http/HttpClientBase.cs | Adds async request helpers and response-stream ownership utilities. |
| Directory.Build.targets | Adds feature defines and formatting adjustments for framework-conditional compilation. |
Comments suppressed due to low confidence (1)
src/Lucene.Net.Replicator/ReplicationClient.cs:528
- The comment in
StopUpdateThread()says signalingstopwill terminate the thread if it is “awaiting the lock”, but thestopevent is only waited on during the sleep (stop.Wait(timeout)), not duringupdateLock.Wait(). WithSemaphoreSlim, signalingstopwon’t unblock a thread waiting to acquireupdateLock. Please update the comment to reflect the actual behavior (or add logic to checkstopbefore/while waiting onupdateLockif that was the intent).
// this will trigger the thread to terminate if it awaits the lock.
// otherwise, if it's in the middle of replication, we wait for it to
// stop.
updateThread.stop.Signal();
try
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@NehanPathan I'll review Copilot's feedback here (as well as run it through a few other models) and will update the PR with any needed changes. We'll proceed with my latest commit for the constructor approach. |
|
@paulirwin I’ll start through the Copilot comments (especially around async flow, disposal, and streaming), and will align with your direction on the constructor approach. Let me know if there are specific areas you’d prefer I focus on. |
|
I picked this back up again, trying to finish it up and ensure all the Copilot feedback is addressed properly, and discovered several likely bugs, particularly around synchronization (reentrant vs non-reentrant primitives, async deadlocks, disposal, task completion exceptions, etc). Additionally, "doing this right" while trying to adhere to some of the design principles discussed here (i.e. having sync and async versions in the same class) is going to result in a maintenance burden, I'm afraid, because it can't simply be done the same way "but just made async." For example, the My current thinking has gone back to thinking that we would actually be better served by splitting out a separate AsyncReplicationClient class, or even moving that support to the extensions project as a Microsoft.Extensions.Hosting BackgroundService. That way we can keep ReplicationClient very close to upstream (with no breaking changes for users), still support IAsyncReplicator in our Replicator library and its implementation in HttpReplicator as primitives for others to use if needed, but allow for our async-based ReplicationClient implementation to follow a different path. I could go either way on whether to make it another class in our Replicator library, or to move to Extensions. Pros for each:
And to clarify and emphasize what I said above: IAsyncReplicator and HttpReplicator's implementation of it would remain in this PR if we choose option B. I'm currently leaning towards that, but open to option A too if there's a strong consensus for that. Also, it should be noted, async support for a replication client was primarily attempted out of consistency with the rest of our async support for replication. It's typically not strictly necessary, since you don't need to worry about hogging a thread for a background job which likely just has one thread anyways. Since this is proving to be more complex, difficult, and disruptive to future porting efforts than anticipated, I think we can pivot to Option B which would allow for a more modern IHostedService approach, still support the asynchronous code, and keep it out of the way for future porting efforts and bug fixes. @NightOwl888 @NehanPathan @Shazwazza Please let me know your thoughts. |
|
@paulirwin After thinking through the synchronization and lifecycle side more carefully, the new direction makes more sense to me as well. Initially I was mostly looking at it from the perspective of “making async work” inside the existing Especially with things like:
Even if we solve those correctly now, it still adds quite a bit of long-term complexity to a class that was originally designed around a synchronous polling/thread model. Keeping I also think this could give us more flexibility if we eventually decide to evolve beyond polling-based replication loops toward more event-driven approaches without complicating the core replication client further. So overall I’m aligned with the new direction, and I’m happy to help with whichever approach we decide to move forward with. |
…lient-side async API, related to #XXXX)
…safe); fix all warnings and nullable issues
…REAM_CANCELLATIONTOKEN defines for .NET 8+ and fix all warnings
…ad of OperationCanceledException
ff27baf to
e38b013
Compare
paulirwin
left a comment
There was a problem hiding this comment.
I removed Async support from ReplicationClient per the discussion in the PR. We will move that to the Extensions repo, perhaps as a BackgroundService.
|
@paulirwin The updated architecture matches the direction we discussed earlier, with IAsyncReplicator remaining in the core library while the async replication client is moved out to the Extensions project. The cleanup around removing the async ReplicationClient implementation looks complete, and I didn't notice any dangling references or inconsistencies after the refactoring. |
Feedback addressed, and re-reviewed by NehanPathan
Fixes:
Fixes #1181
Summary of the changes:
Implemented
IAsyncReplicatorfor non-blocking replication operations.Description
This PR introduces an async Task-based API for the replication support, allowing operations such as checking for updates, obtaining files, and releasing sessions to be executed asynchronously.
Key changes:
IAsyncReplicatorinterface.HttpReplicatorto implement async versions of the operations (CheckForUpdateAsync,ObtainFileAsync,ReleaseAsync,PublishAsync).This avoids synchronous HTTP calls that could deadlock or cause performance issues, while keeping
IReplicationHandlersynchronous, as Lucene.NET APIs currently do not have async equivalents.Additional context:
This implementation has been tested and works successfully when using
UpdateNowAsyncin the GSoC project by referencing the Lucene.NET repository in the GSoC extensions project.UPDATE by @paulirwin 6/24/2026: Async support for ReplicationClient has been removed, per discussion. We will reintroduce it separately in the Extensions repo/packages.