NightOwl888 commented on code in PR #1170:
URL: https://github.com/apache/lucenenet/pull/1170#discussion_r2312334781
##########
src/Lucene.Net.Replicator/Support/Http/Abstractions/IReplicationResponse.cs:
##########
@@ -42,5 +44,12 @@ public interface IReplicationResponse
/// Flushes the reponse to the underlying response stream.
/// </summary>
void Flush();
+
+ /// <summary>
+ /// Flushes the response to the underlying response stream
asynchronously.
+ /// </summary>
+ /// <param name="cancellationToken">Optional cancellation
token.</param>
+ /// <returns>A task representing the asynchronous operation.</returns>
+ Task FlushAsync(CancellationToken cancellationToken = default);
Review Comment:
This is the only thing I was unsure about so I withheld comment until I
could do some research.
While async HTTP is supported on all server stacks as far back as we care to
support, the direction things are headed will likely end up with no synchronous
HTTP support on stacks in the future. So, it is not reasonable for there to be
a `Flush()` implementation on every implementation of `IReplicationResponse`. A
technology lacking synchronous support would have no good options for
implementing it.
### Throwing an exception
```c#
void Flush() => throw new NotSupportedException();
```
This seems like a hack and underscores the fundamental issue with the
interface design.
### Faking it
```c#
void Flush() => FlushAsync().GetAwaiter().GetResult();
```
Also not a good option, since it can lead to deadlocks.
## Options
### Option A: Keep both, but make Flush() optional
- Document that `Flush()` may throw `NotSupportedException` if the
underlying server disallows sync I/O.
- This makes the abstraction maximally portable, but puts burden on
consumers to handle failure.
### Option B: Move Flush() to a separate interface
- Keeps async-first in the main abstraction.
- Allows servers that can’t support sync to just implement the async
interface.
- Clear separation: consumers who truly need sync must check for it
explicitly.
There are a couple of different ways to do this:
#### Inheritance
```c#
public interface ISynchronousReplicationResponse : IReplicationResponse
{
void Flush();
}
```
This means that if someone only wanted a synchronous `Flush()` they would
also be required to implement `FlushAsync()`.
#### Separate interfaces
```c#
public interface IReplicationResponse
{
int StatusCode { get; set; }
Stream Body { get; }
void Flush();
}
public interface IAsyncReplicationResponse
{
int StatusCode { get; set; }
Stream Body { get; }
Task FlushAsync(CancellationToken cancellationToken = default);
}
```
This one has my vote. It cleanly separates the sync and async workflows so a
server could implement one, the other, or both.
The naming convention could be `ISynchronousReplicationResponse` and
`IReplicationResponse`, which might be better if we end up dropping the former
in the future. But in the BCL, there are interfaces such as `IAsyncDisposable`
and `IAsyncEnumerable<T>`, but none specifying synchronous I/O, so the above
convention is more in line with the BCL.
I looked through the usages, and there are no issues at all in the
production code. In tests, we can just use `IAsyncReplicationService` and
switch if we need to:
```c#
if (service is IReplicationService synchronousService)
synchronousService.Perform(...);
else
throw new NotSupportedException("The service doesn't support synchronous
I/O");
```
---------------------
Whatever option we choose, we should give the same treatment to
`IReplicationService.Perform()` and `IReplicationService.PerformAsync()` for
consistency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
