NightOwl888 commented on code in PR #1170:
URL: https://github.com/apache/lucenenet/pull/1170#discussion_r2304919146
##########
src/Lucene.Net.Replicator/SessionToken.cs:
##########
@@ -112,6 +114,33 @@ public void Serialize(DataOutputStream writer)
}
}
+ /// <summary>
+ /// Asynchronously serialize the token data for communication between
server and client.
+ /// </summary>
+ /// <param name="output">The <see cref="Stream"/> to write the token
data to.</param>
+ /// <param name="cancellationToken">A cancellation token to observe
while waiting for the flush to complete.</param>
+ /// <returns>A task representing the asynchronous operation.</returns>
+ public async Task SerializeAsync(Stream output, CancellationToken
cancellationToken = default)
+ {
+ using var writer = new DataOutputStream(output);
+ writer.WriteUTF(Id);
Review Comment:
> ## @NightOwl888
> I wanted to update you regarding the low-level UTF and integer stream
methods. I initially tried using `Span<byte>` / `ReadOnlySpan<byte>` to reduce
allocations and align with BCL-style patterns.
>
> However, this causes compiler issues in the current project setup. The
async `ReadAsync`/`WriteAsync` overloads with `Span<byte>` /
`ReadOnlySpan<byte>` cannot be used, producing errors like:
>
> * `No overload for method 'WriteAsync' takes 2 arguments`
> * `Cannot convert from 'Span<byte>' to 'Memory<byte>'`
> * `Feature 'ref and unsafe in async and iterator methods' is not available
in C# 11.0`
>
> Because of this, I propose we **use `byte[]` for all Read/Write methods**,
including the UTF methods. This approach is consistent with the existing
synchronous Read/Write helpers. The **byte[] approach works perfectly**, all
tests have been run, and everything functions as expected. We can still include
the updated UTF exception handling (`EncoderFallbackException` /
`DecoderFallbackException`) as per BCL conventions.
>
> This ensures compatibility with the current codebase without causing any
compiler issues.
>
> Any suggestion to solve issue or thoughts on this byte approach??, and I
can finalize the changes accordingly.
Understood. This overload of `WriteAsync` is not available prior to .NET
Core. The actual method isn't very complicated:
https://github.com/dotnet/runtime/blob/1d1bf92fcf43aa6981804dc53c5174445069c9e4/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs#L740-L762
However, it is clear after looking at the implementation why it accepts
`ReadOnlyMemory<byte>` instead of `ReadOnlySpan<byte>` - because the latter has
no way to convert to an array even if it is on the heap.
So, don't worry about using spans for the time being. It seems that
Microsoft hasn't fully implemented support for them in `Stream`. Go ahead and
use one of the `byte[]` overloads.
--
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: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
