Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 6, 2025

Fix JsonSerializer.Deserialize for ReadOnlyMemory and Memory in streaming scenarios

Problem

When deserializing ReadOnlyMemory<T> or Memory<T> from a stream, the converters incorrectly return early when resuming from continuation state if the current token is null, treating it as a root-level null instead of a null array element.

Solution

Changed the null check in OnTryRead to use state.Current.ObjectState == StackFrameObjectState.None instead of !state.IsContinuation. This properly distinguishes between:

  1. Initial call where root value is null (ObjectState.None) - should return early with default
  2. Resumption during array processing where reader is at a null element (ObjectState != None) - should continue to base implementation

Changes made:

  • ReadOnlyMemoryConverter.cs: Changed line 21 from if (reader.TokenType is JsonTokenType.Null && !state.IsContinuation) to if (reader.TokenType is JsonTokenType.Null && state.Current.ObjectState == StackFrameObjectState.None)
  • MemoryConverter.cs: Changed line 21 from if (reader.TokenType is JsonTokenType.Null && !state.IsContinuation) to if (reader.TokenType is JsonTokenType.Null && state.Current.ObjectState == StackFrameObjectState.None)
  • CollectionTests.Memory.cs: Added two comprehensive regression tests:
    • DeserializeReadOnlyMemoryFromStreamWithManyNulls: Tests with 200 null elements and DefaultBufferSize=1
    • DeserializeMemoryFromStreamWithManyNulls: Tests with 200 null elements and DefaultBufferSize=1
    • Uses Enumerable.Repeat for clean test data generation

Testing

  • All 49,800 System.Text.Json tests pass
  • New tests specifically validate the fix works with extreme buffer constraints that force resumptions
  • Tests verify 200 consecutive nulls are correctly deserialized

Technical Details

The fix uses ObjectState == None instead of !IsContinuation because:

  • When ObjectState == None, we're at the very beginning before any processing has started - a null token means the root value is null
  • When ObjectState != None, we've already started processing the collection - a null token is likely a null element in the array, not a root-level null

This properly handles resumption at arbitrary reader positions during streaming deserialization with small buffer sizes.

Security Summary

No security vulnerabilities were introduced or discovered during this change.

Fixes #118346

Original prompt

This section details on the original issue you should resolve

<issue_title>JsonSerializer.Deserialize fails to deserialize to {ReadOnly}Memory</issue_title>
<issue_description>### Description

When trying to deserialize a JSON to {ReadOnly}Memory<T>, JsonSerializer.Deserialize fails for some perfectly valid JSON files.

The following code prints Deserialization failed miserably!:

        using FileStream fileStream = File.OpenRead("Names.json");
        ReadOnlyMemory<NameRecord> nameRecords = JsonSerializer.Deserialize<ReadOnlyMemory<NameRecord>>(fileStream);
        if (nameRecords.Length is 0)
        {
            Console.WriteLine("Deserialization failed miserably!");
        }
        else
        {
            Console.WriteLine("Deserialization succeeded!");
        }

While the following code prints Deserialization succeeded!:

        using FileStream fileStream = File.OpenRead("Names.json");
        ReadOnlyMemory<NameRecord> nameRecords = JsonSerializer.Deserialize<NameRecord[]>(fileStream);
        if (nameRecords.Length is 0)
        {
            Console.WriteLine("Deserialization failed miserably!");
        }
        else
        {
            Console.WriteLine("Deserialization succeeded!");
        }

Potentially related to #118346


Should we just not use {ReadOnly}Memory as a deserialization target? Is its usage some kind of an edge case that is not well tested? I was under the impression that if I don't strictly need a List<T> or T[], deserializing a JSON file to ReadOnlyMemory<T> would have some performance benefits (like allocating less) but it causing all these issues for random JSON files makes me wonder whether I am missing a memo where its usage is discouraged for JSON deserialization.

Reproduction Steps

  1. Run ReadOnlyMemoryDeserializationBug.zip
  2. Notice it prints Deserialization failed miserably!
  3. Change JsonSerializer.Deserialize<ReadOnlyMemory<NameRecord>>(fileStream) to JsonSerializer.Deserialize<NameRecord[]>(fileStream) and re-run it.
  4. Notice it prints Deserialization succeeded!

Expected behavior

JsonSerializer.Deserialize<ReadOnlyMemory<T>> should be able to deserialize everything JsonSerializer.Deserialize<T[]> can deserialize.

Actual behavior

JsonSerializer.Deserialize<ReadOnlyMemory<T>> fails to deserialize some JSON files that JsonSerializer.Deserialize<T[]> deserializes just fine.

Regression?

Partially. For some JSON files .NET 8 does not fail but .NET 9 fails. For some other JSON files .NET 8 fails with an (incorrect) exception but .NET 9 fails silently. The attached repro project showcases where both .NET versions fail.

Known Workarounds

Deserializing to List<T> or T[] works fine

Configuration

Which version of .NET is the code running on?

.NET 9

What OS and version, and what distro if applicable?

Windows 10 22H2

What is the architecture (x64, x86, ARM, ARM64)?

x64

Other information

No response</issue_description>

<agent_instructions>I can confirm that this is a bug in the built in Memory and ReadOnlyMemory converters when performing streaming serialization. The null check at the start of the OnTryRead method:

if (reader.TokenType is JsonTokenType.Null)
{
value = default;
return true;
}

Does not take resumption into account and returns early if the converter is attempting to resume when the current (nested) JSON token happens to be null. A fix should additionally include a state.IsContinuation check in each of the two converters.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@eiriktsarpalis I can confirm that this is a bug in the built in Memory and ReadOnlyMemory converters when performing streaming serialization. The null check at the start of the `OnTryRead` method:

if (reader.TokenType is JsonTokenType.Null)
{
value = default;
return true;
}

Does not take resumption into account and returns early if the converter is attempting to resume when the current (nested) JSON token happens to be null. A fix should additionally include a state.IsContinuation check in each of the two converters.</comment_new>


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI self-assigned this Nov 6, 2025
Add IsContinuation check to prevent early return when resuming from continuation state with null tokens

Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix JsonSerializer to deserialize ReadOnlyMemory correctly Fix ReadOnlyMemory/Memory deserialization during stream continuation with null elements Nov 6, 2025
Copilot finished work on behalf of eiriktsarpalis November 6, 2025 16:39
Copilot AI requested a review from eiriktsarpalis November 6, 2025 16:39
…umption

Changed from !state.IsContinuation to state.Current.ObjectState == StackFrameObjectState.None for proper resumption handling. Added tests with 200 nulls and DefaultBufferSize=1 as requested.

Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Simplified null array generation using Enumerable.Repeat instead of Array.Fill as suggested in code review.

Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Copilot AI requested a review from eiriktsarpalis November 7, 2025 16:51
Copilot finished work on behalf of eiriktsarpalis November 7, 2025 16:51
@eiriktsarpalis eiriktsarpalis marked this pull request as ready for review November 7, 2025 18:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in the deserialization of Memory<T> and ReadOnlyMemory<T> when deserializing from streams containing arrays with many null elements. The issue occurred because the converters would incorrectly treat null array elements as null collections during streaming deserialization with resumptions.

  • Modified MemoryConverter<T> and ReadOnlyMemoryConverter<T> to check state.Current.ReturnValue before treating null tokens as null collections
  • Added regression tests for both Memory<string?> and ReadOnlyMemory<string?> deserialization from streams with 200 null elements
  • Added JsonSerializable attributes to register Memory<string> and ReadOnlyMemory<string> types for source generation tests

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
ReadOnlyMemoryConverter.cs Fixed null handling to distinguish between null collection and null array elements during streaming deserialization
MemoryConverter.cs Fixed null handling to distinguish between null collection and null array elements during streaming deserialization
CollectionTests.Memory.cs Added regression tests for deserializing Memory/ReadOnlyMemory with many null elements from streams
CollectionTests.cs (SourceGeneration) Registered Memory and ReadOnlyMemory types for source generation test contexts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants