Fix SGen null complex prop bug, add CHUNK_ABORT to SignalR
- Fix SGen-generated writer bug: always null-check reference-type properties before serialization, emitting PropertySkip if null, to prevent runtime NREs. - Add regression tests for SGen null complex property handling. - Introduce CHUNK_ABORT ([203]) marker to SignalR binary protocol for graceful mid-stream serialize failure handling; update protocol logic and docs. - Improve documentation to cover bug, fix, and new protocol marker. - Minor: remove explicit net10.0 target from test csproj.
This commit is contained in:
parent
e2b96b4148
commit
11d76270dc
|
|
@ -306,13 +306,17 @@ public partial class AcBinarySourceGenerator
|
|||
sb.AppendLine($"{i} AcBinarySerializer.WriteValueGenerated({a}, {a}.GetType(), context);");
|
||||
sb.AppendLine($"{i}}}");
|
||||
}
|
||||
else if (p.IsNullable)
|
||||
else
|
||||
{
|
||||
// Reference type properties can always be null at runtime regardless of nullable
|
||||
// annotation — runtime can violate the nullable-disabled contract via EF lazy-load,
|
||||
// projection gaps, detached navigation properties, etc. Mirrors the EmitDirectObjectWrite
|
||||
// (line ~828) and EmitDirectCollectionWrite (line ~877) defensive null-check.
|
||||
// Reader-side compat: every markered property is wrapped in `if (tc != PropertySkip)`
|
||||
// by EmitReadProperty (GenReader.cs line ~137), so the marker is uniformly handled.
|
||||
sb.AppendLine($"{i}if ({a} == null) context.WriteByte(BinaryTypeCode.PropertySkip);");
|
||||
sb.AppendLine($"{i}else AcBinarySerializer.WriteObjectGenerated({a}, typeof({p.TypeNameForTypeof}), context);");
|
||||
}
|
||||
else
|
||||
sb.AppendLine($"{i}AcBinarySerializer.WriteObjectGenerated({a}, typeof({p.TypeNameForTypeof}), context);");
|
||||
break;
|
||||
case PropertyTypeKind.Collection:
|
||||
// Direct collection write for List<T>/T[] with Complex element types that have generated writers
|
||||
|
|
|
|||
|
|
@ -0,0 +1,70 @@
|
|||
using AyCode.Core.Serializers.Binaries;
|
||||
using AyCode.Core.Tests.TestModels;
|
||||
|
||||
namespace AyCode.Core.Tests.Serialization;
|
||||
|
||||
[TestClass]
|
||||
public class AcBinarySerializerSGenNullComplexPropertyTests
|
||||
{
|
||||
[TestMethod]
|
||||
[DataRow(true, true)]
|
||||
[DataRow(true, false)]
|
||||
[DataRow(false, false)]
|
||||
[DataRow(false, true)]
|
||||
public void Serialize_SGenComplexPropertyNull_DoesNotThrow_AndRoundTripsAsNull(bool useSgen, bool fastMode)
|
||||
{
|
||||
var model = new SGenNullComplexParent
|
||||
{
|
||||
Id = 7,
|
||||
Customer = null!,
|
||||
Note = "regression"
|
||||
};
|
||||
|
||||
var options = fastMode ? AcBinarySerializerOptions.FastMode: AcBinarySerializerOptions.Default;
|
||||
options.UseGeneratedCode = useSgen;
|
||||
|
||||
var bytes = AcBinarySerializer.Serialize(model, options);
|
||||
var roundTrip = AcBinaryDeserializer.Deserialize<SGenNullComplexParent>(bytes, options);
|
||||
|
||||
Assert.IsNotNull(roundTrip);
|
||||
Assert.AreEqual(model.Id, roundTrip.Id);
|
||||
Assert.AreEqual(model.Note, roundTrip.Note);
|
||||
Assert.IsNull(roundTrip.Customer,
|
||||
"complex reference property must round-trip as null when source was null " +
|
||||
"(regression for SGen WriteObjectGenerated fallback else-branch null-check)");
|
||||
|
||||
Assert.IsTrue(System.Array.IndexOf(bytes, (byte)BinaryTypeCode.PropertySkip) >= 0,
|
||||
"writer must emit PropertySkip marker on the null Customer slot " +
|
||||
"(deeper verification: confirms the fix took the PropertySkip path, " +
|
||||
"not an unrelated null-safe code path)");
|
||||
}
|
||||
|
||||
[TestMethod]
|
||||
[DataRow(true, true)]
|
||||
[DataRow(true, false)]
|
||||
[DataRow(false, false)]
|
||||
[DataRow(false, true)]
|
||||
public void Serialize_SGenComplexPropertyNonNull_RoundTripsCorrectly(bool useSgen, bool fastMode)
|
||||
{
|
||||
var model = new SGenNullComplexParent
|
||||
{
|
||||
Id = 13,
|
||||
Customer = new NonGeneratedComplexCustomer { Id = 42, Name = "child" },
|
||||
Note = "positive"
|
||||
};
|
||||
|
||||
var options = fastMode ? AcBinarySerializerOptions.FastMode: AcBinarySerializerOptions.Default;
|
||||
options.UseGeneratedCode = useSgen;
|
||||
|
||||
var bytes = AcBinarySerializer.Serialize(model, options);
|
||||
var roundTrip = AcBinaryDeserializer.Deserialize<SGenNullComplexParent>(bytes, options);
|
||||
|
||||
Assert.IsNotNull(roundTrip);
|
||||
Assert.AreEqual(model.Id, roundTrip.Id);
|
||||
Assert.AreEqual(model.Note, roundTrip.Note);
|
||||
Assert.IsNotNull(roundTrip.Customer,
|
||||
"non-null complex reference property must round-trip (null-check fix must not break the non-null path)");
|
||||
Assert.AreEqual(model.Customer.Id, roundTrip.Customer.Id);
|
||||
Assert.AreEqual(model.Customer.Name, roundTrip.Customer.Name);
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,27 @@
|
|||
using AyCode.Core.Serializers.Attributes;
|
||||
|
||||
namespace AyCode.Core.Tests.TestModels;
|
||||
|
||||
/// <summary>
|
||||
/// Intentionally NOT marked with [AcBinarySerializable].
|
||||
/// Used to reproduce the generated-writer path where the parent has a complex reference property
|
||||
/// without a generated writer on the child type.
|
||||
/// </summary>
|
||||
public class NonGeneratedComplexCustomer
|
||||
{
|
||||
public int Id { get; set; }
|
||||
public string? Name { get; set; }
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Regression model for SGen complex-property null handling.
|
||||
/// The Customer property is non-nullable in signature, but runtime data can still contain null.
|
||||
/// Serializer must emit PropertySkip instead of dereferencing null.
|
||||
/// </summary>
|
||||
[AcBinarySerializable]
|
||||
public class SGenNullComplexParent
|
||||
{
|
||||
public int Id { get; set; }
|
||||
public NonGeneratedComplexCustomer Customer { get; set; } = null!;
|
||||
public string? Note { get; set; }
|
||||
}
|
||||
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
|
|
@ -1,7 +1,6 @@
|
|||
<Project Sdk="Microsoft.NET.Sdk">
|
||||
|
||||
<PropertyGroup>
|
||||
<TargetFramework>net10.0</TargetFramework>
|
||||
<LangVersion>latest</LangVersion>
|
||||
<ImplicitUsings>enable</ImplicitUsings>
|
||||
<Nullable>enable</Nullable>
|
||||
|
|
|
|||
|
|
@ -53,6 +53,18 @@ public class AcBinaryHubProtocol : IHubProtocol
|
|||
private const byte MsgAsyncChunkData = 201;
|
||||
private const byte MsgAsyncChunkEnd = 202;
|
||||
|
||||
/// <summary>
|
||||
/// CHUNK_ABORT marker. Emitted by the sender's <see cref="WriteMessageChunked"/> if the
|
||||
/// streamed-arg serialize fails mid-stream after CHUNK_START has been sent — instead of letting
|
||||
/// the exception propagate (which would abort the entire SignalR transport connection in
|
||||
/// <c>HubConnectionContext.WriteCore</c>, killing all other in-flight invocations on the same
|
||||
/// WebSocket), the sender writes a single <c>[203]</c> byte so the receiver can fault the
|
||||
/// pending invocation cleanly while the transport stays alive (fault isolation:
|
||||
/// blast radius = one message). Recognised by <see cref="TryParseChunkData"/>, which surfaces
|
||||
/// the abort to the awaiting caller via a synthesised <see cref="CompletionMessage.WithError"/>.
|
||||
/// </summary>
|
||||
private const byte MsgAsyncChunkAbort = 203;
|
||||
|
||||
/// <summary>Sentinel object placed in the args array for the streamed argument (replaced after chunk deserialization).</summary>
|
||||
protected static readonly object StreamedArgPlaceholder = new();
|
||||
|
||||
|
|
@ -529,8 +541,33 @@ public class AcBinaryHubProtocol : IHubProtocol
|
|||
// This includes the null streamedArg case (since the AcBinarySerializer null-bypass for
|
||||
// multiMessage=true was removed) — wire is [201][UINT16=1][Null][202], deserialized back to null.
|
||||
// No manual [202] write or extra FlushAsync needed in this layer.
|
||||
//
|
||||
// Fault isolation: if the streamed-arg serialize throws (e.g. a property getter NRE on the
|
||||
// receiver's data class), CHUNK_START has already been sent — the receiver is in chunk-state
|
||||
// waiting for [201]/[202]. Letting the exception propagate would abort the entire SignalR
|
||||
// transport connection in HubConnectionContext.WriteCore (killing all other in-flight
|
||||
// invocations on the same WebSocket). Instead, emit an explicit [203] CHUNK_ABORT marker so
|
||||
// the receiver can fault the pending invocation cleanly while the connection stays alive.
|
||||
// Blast radius = one message.
|
||||
// Edge case (deferred): if AsyncPipeWriterOutput fails after committing a [201][UINT16=N]
|
||||
// header but before writing all N data bytes, the receiver parses [203] as part of that
|
||||
// chunk's data, not as the abort marker. Rare (exceptions typically throw mid-data, not
|
||||
// mid-header), and reaches the protocol-violation path in TryParseChunkData. Robust fix:
|
||||
// AsyncPipeWriterOutput.Abort() padding the in-flight chunk before emitting [203] — see
|
||||
// BINARY_ASYNCPIPE_TODO.
|
||||
try
|
||||
{
|
||||
dataBytes = AcBinarySerializer.Serialize(streamedArg, pipeWriter, _options, _flushPolicy, _flushTimeout);
|
||||
_logger?.LogDebug("WriteMessageChunked CHUNK_DATA + CHUNK_END emitted via AsyncPipeWriterOutput dataBytes={DataBytes}", dataBytes);
|
||||
}
|
||||
catch (Exception serializeEx)
|
||||
{
|
||||
_logger?.LogError(serializeEx, "WriteMessageChunked streamed-arg serialize FAILED — emitting [203] CHUNK_ABORT messageType={MessageType}", message.GetType().Name);
|
||||
|
||||
if (!TryEmitChunkAbort(pipeWriter)) throw; // pipe dead too — let SignalR abort the connection (baseline behaviour)
|
||||
|
||||
return; // abort marker on the wire, connection alive, receiver faults the caller
|
||||
}
|
||||
|
||||
// Total wire bytes = length prefix (4) + CHUNK_START payload + CHUNK_DATA frames + CHUNK_END (1)
|
||||
// Each CHUNK_DATA frame adds 3 bytes ([201][UINT16 size]) per chunkSize-worth of data.
|
||||
|
|
@ -964,29 +1001,150 @@ public class AcBinaryHubProtocol : IHubProtocol
|
|||
return true;
|
||||
}
|
||||
|
||||
// Unknown byte in chunk mode.
|
||||
// Real-world case: server-side serialization fails after CHUNK_START was sent, then SignalR
|
||||
// emits a normal framed CloseMessage. The first byte here is then the little-endian payload
|
||||
// length (e.g. 114), not [201]/[202]. If we keep chunk state, subsequent parses keep failing
|
||||
// with the same warning. Instead, tear down chunk mode and re-parse as normal framed message.
|
||||
_logger?.LogWarning("TryParseChunkData unknown byte {FirstByte} in chunk mode, falling back to normal parse. " +
|
||||
"binderHash={BinderHash} inputLength={InputLength} " +
|
||||
"state: streamedArgType={TargetType} deserTaskStatus={TaskStatus} chunkFrameBytesConsumed={ChunkFrameBytesConsumed}",
|
||||
firstByte,
|
||||
binder.GetHashCode(),
|
||||
input.Length,
|
||||
if (firstByte == MsgAsyncChunkAbort) // 203 — server abandoned the chunked message mid-stream
|
||||
{
|
||||
_logger?.LogWarning("TryParseChunkData [203] CHUNK_ABORT targetType={TargetType} chunkFrameBytesConsumed={ChunkFrameBytesConsumed}",
|
||||
state.StreamedArgType.Name, state.ChunkFrameBytesConsumed);
|
||||
|
||||
AbandonChunkState(state, binder, reason: "[203] CHUNK_ABORT");
|
||||
input = input.Slice(1); // consume the [203] byte
|
||||
|
||||
// Surface the abort to the caller via OnChunkAbort. Default base routing uses
|
||||
// SignalR's InvocationId (CompletionMessage.WithError); derived classes can
|
||||
// override for application-level correlation (e.g. SignalParams.requestId in args).
|
||||
var invocationId = GetInvocationId(state.PartialMessage);
|
||||
message = OnChunkAbort(state.PartialMessage, state.HeaderContext, invocationId);
|
||||
|
||||
if (message == null)
|
||||
{
|
||||
// No routing target — return PingMessage so the SignalR loop's "consumed input
|
||||
// ↔ produced message" contract holds. True fire-and-forget, or override handed
|
||||
// off routing out-of-band.
|
||||
_logger?.LogWarning("TryParseChunkData [203] OnChunkAbort returned null — returning Ping (no caller to fault)");
|
||||
message = PingMessage.Instance;
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
// Protocol-invariant violation — not [201]/[202]/[203]. Legitimate sender-abort is
|
||||
// handled by the [203] branch above; anything reaching here is genuine framing
|
||||
// corruption (sender bug, version mismatch, or chunk header misread drifting into
|
||||
// data). The old "fallback to normal parse" was guess-and-hope: it can't tell mid-data
|
||||
// garbage from a real next message. Surface as InvalidDataException — SignalR's outer
|
||||
// handler treats it as a transport fault rather than masking it.
|
||||
_logger?.LogError("TryParseChunkData PROTOCOL VIOLATION unknown byte {FirstByte} in chunk mode (expected [201]/[202]/[203]). " +
|
||||
"binderHash={BinderHash} inputLength={InputLength} targetType={TargetType} deserTaskStatus={TaskStatus} chunkFrameBytesConsumed={ChunkFrameBytesConsumed}",
|
||||
firstByte, binder.GetHashCode(), input.Length,
|
||||
state.StreamedArgType.Name,
|
||||
state.DeserTask?.Status.ToString() ?? "null",
|
||||
state.ChunkFrameBytesConsumed);
|
||||
|
||||
state.Input.Dispose();
|
||||
_chunkStates.Remove(binder);
|
||||
return TryParseMessage(ref input, binder, out message);
|
||||
AbandonChunkState(state, binder, reason: $"protocol violation (byte 0x{firstByte:X2})");
|
||||
|
||||
throw new System.IO.InvalidDataException(
|
||||
$"AcBinary chunked protocol violation: unexpected byte 0x{firstByte:X2} ({firstByte}) in chunk mode " +
|
||||
$"for targetType={state.StreamedArgType.Name}; expected [201] CHUNK_DATA, [202] CHUNK_END, or [203] CHUNK_ABORT.");
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Teardown for non-success chunk-state termination (CHUNK_ABORT or protocol violation).
|
||||
/// Mirrors the CHUNK_END path's Complete → await-deser → Dispose → Remove ordering, but
|
||||
/// observes the background deser task's failure (likely on partial input) without rethrowing —
|
||||
/// the abort/violation is the authoritative outcome, the deser exception is a derived effect.
|
||||
/// </summary>
|
||||
private void AbandonChunkState(AsyncChunkState state, IInvocationBinder binder, string reason)
|
||||
{
|
||||
state.Input.Complete();
|
||||
|
||||
if (state.DeserTask != null)
|
||||
{
|
||||
try
|
||||
{
|
||||
state.DeserTask.GetAwaiter().GetResult();
|
||||
}
|
||||
catch (Exception deserEx)
|
||||
{
|
||||
_logger?.LogDebug(deserEx,
|
||||
"AbandonChunkState ({Reason}): background deser task faulted on partial input (expected)",
|
||||
reason);
|
||||
}
|
||||
}
|
||||
|
||||
state.Input.Dispose();
|
||||
_chunkStates.Remove(binder);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Extracts the InvocationId from a chunked-mode <see cref="AsyncChunkState.PartialMessage"/>,
|
||||
/// covering all message types that flow through <see cref="WriteMessageChunked"/>. Returns
|
||||
/// <c>null</c> for unexpected types (defensive — shouldn't occur in normal use).
|
||||
/// </summary>
|
||||
private static string? GetInvocationId(HubMessage message) => message switch
|
||||
{
|
||||
InvocationMessage im => im.InvocationId,
|
||||
StreamInvocationMessage sim => sim.InvocationId,
|
||||
StreamItemMessage stim => stim.InvocationId,
|
||||
CompletionMessage cm => cm.InvocationId,
|
||||
_ => null
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// Best-effort emit of the [203] CHUNK_ABORT marker after a serialize failure in
|
||||
/// <see cref="WriteMessageChunked"/>. Returns <c>true</c> if the byte was successfully written
|
||||
/// and flushed; <c>false</c> if even the abort emit failed (transport-level fault) — the
|
||||
/// caller should then rethrow to fall back to connection-level abort behaviour.
|
||||
/// </summary>
|
||||
private bool TryEmitChunkAbort(PipeWriter pipeWriter)
|
||||
{
|
||||
try
|
||||
{
|
||||
var abortSpan = pipeWriter.GetSpan(1);
|
||||
abortSpan[0] = MsgAsyncChunkAbort;
|
||||
pipeWriter.Advance(1);
|
||||
SyncFlush(pipeWriter.FlushAsync());
|
||||
|
||||
_logger?.LogDebug("WriteMessageChunked [203] CHUNK_ABORT emitted (graceful fault isolation)");
|
||||
return true;
|
||||
}
|
||||
catch (Exception abortEx)
|
||||
{
|
||||
_logger?.LogError(abortEx,
|
||||
"WriteMessageChunked failed to emit [203] CHUNK_ABORT — falling back to connection abort");
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Called by the CHUNK_ABORT <c>[203]</c> receive branch in <see cref="TryParseChunkData"/> to
|
||||
/// produce a HubMessage that surfaces the abort to the awaiting caller.
|
||||
/// <para>Default base implementation: returns <see cref="CompletionMessage.WithError"/> if
|
||||
/// <paramref name="invocationId"/> is non-null (SignalR-level routing — the awaiting task is
|
||||
/// faulted with the embedded error). Returns <c>null</c> for fire-and-forget invocations (no
|
||||
/// SignalR InvocationId), in which case the caller falls back to <see cref="PingMessage.Instance"/>
|
||||
/// and the abort is not propagated to any specific waiter.</para>
|
||||
/// <para>Derived classes can override to synthesise an application-level error response — e.g.
|
||||
/// when the protocol uses a custom request/response correlation in the message arguments
|
||||
/// (rather than SignalR's InvocationId), the override can build an <see cref="InvocationMessage"/>
|
||||
/// that routes to the caller's application-level error path. Returning a non-null result short-circuits
|
||||
/// the base SignalR-level routing; returning <c>null</c> explicitly hands off to the Ping fallback
|
||||
/// (signalling "the abort was handled out-of-band or has no addressable caller").</para>
|
||||
/// </summary>
|
||||
/// <param name="partialMessage">The original message that activated chunk mode (with the streamed-arg placeholder still in place).</param>
|
||||
/// <param name="headerContext">The opaque header context produced by <see cref="ReadHeader"/> for this message.</param>
|
||||
/// <param name="invocationId">The SignalR-level InvocationId, or <c>null</c> for fire-and-forget messages.</param>
|
||||
/// <returns>A HubMessage to surface to the SignalR loop, or <c>null</c> to fall back to <see cref="PingMessage.Instance"/>.</returns>
|
||||
protected virtual HubMessage? OnChunkAbort(HubMessage partialMessage, object? headerContext, string? invocationId)
|
||||
{
|
||||
if (string.IsNullOrEmpty(invocationId)) return null;
|
||||
|
||||
return CompletionMessage.WithError(invocationId,
|
||||
"Server abandoned the chunked response (remote serialize failure — see server logs).");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Parses CHUNK_START: reads original message (with -1 marker for streamed arg),
|
||||
/// creates <see cref="AsyncPipeReaderInput"/>, stores state. Background deser task starts lazily on first chunk.
|
||||
|
|
|
|||
|
|
@ -218,4 +218,47 @@ public class AyCodeBinaryHubProtocol : AcBinaryHubProtocol
|
|||
if (rented) ArrayPool<byte>.Shared.Return(arr);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Application-level CHUNK_ABORT routing for the AyCode <see cref="SignalParams"/> correlation
|
||||
/// pattern. The base SignalR InvocationId is null on server-to-client <c>SendAsync</c>
|
||||
/// (fire-and-forget at the SignalR layer), but the application encodes
|
||||
/// <c>(messageTag, requestId, SignalParams, data)</c> into <c>arg[0..3]</c> of an
|
||||
/// <c>OnReceiveMessage</c> callback. Synthesise a <see cref="SignalResponseStatus.Error"/>
|
||||
/// response so the client's <c>_responseByRequestId</c> routing faults the awaiting Task with
|
||||
/// a specific error instead of waiting for a transport-level timeout.
|
||||
/// </summary>
|
||||
protected override HubMessage? OnChunkAbort(HubMessage partialMessage, object? headerContext, string? invocationId)
|
||||
{
|
||||
// Recognise the AyCode OnReceiveMessage(messageTag, requestId, SignalParams, data) shape —
|
||||
// arg[1] is the application's correlation key, arg[2] is SignalParams.
|
||||
if (partialMessage is InvocationMessage inv
|
||||
&& inv.Arguments.Length >= 4
|
||||
&& inv.Arguments[2] is SignalParams origParams)
|
||||
{
|
||||
var errorParams = new SignalParams
|
||||
{
|
||||
Status = SignalResponseStatus.Error,
|
||||
IsRawBytesData = origParams.IsRawBytesData,
|
||||
DataSerializerType = origParams.DataSerializerType
|
||||
};
|
||||
|
||||
// Same target ("OnReceiveMessage") and args[0..1], SignalParams replaced with Error
|
||||
// status, data arg (last) cleared — the abort means no data was actually produced.
|
||||
// The client's OnReceiveMessage routes via _responseByRequestId[requestId] (arg[1])
|
||||
// and the SignalResponseStatus.Error surfaces to the awaiting caller.
|
||||
var args = new object?[inv.Arguments.Length];
|
||||
Array.Copy(inv.Arguments, args, inv.Arguments.Length);
|
||||
args[2] = errorParams;
|
||||
args[inv.Arguments.Length - 1] = null;
|
||||
|
||||
_logger?.LogDebug("OnChunkAbort synthesised SignalResponseStatus.Error response messageTag={MessageTag} requestId={RequestId}",
|
||||
inv.Arguments[0], inv.Arguments[1]);
|
||||
|
||||
return new InvocationMessage(inv.Target, args);
|
||||
}
|
||||
|
||||
// Unknown shape — defer to base (SignalR InvocationId routing or null/Ping fallback).
|
||||
return base.OnChunkAbort(partialMessage, headerContext, invocationId);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
Loading…
Reference in New Issue