Sync stream serialization not handling IAsyncEnumerable correctly

[eiriktsarpalis]

While adding new tests to System.Text.Json I discovered an issue with the handling of IAsyncEnumerable values in the new sync stream serialization APIs. It would seem like IAsyncEnumerableConverter's sync serialization detection logic is not adequate in identifying the new methods: in debug builds this results in a failed debug assertion whereas release builds end up serializing IAE values successfully, since they accidentally perform sync-over-async enumeration of the source enumerable. We should try to fix this, I can see a couple of options here:

1. Have the converter detect and fail on sync stream serialization methods. Users that rely on the current behavior might regard this as a breaking change.
2. Double down on the existing (accidental) behavior and use sync-over-sync for the sync stream serialization methods. Should we consider allowing the same behavior for non-stream sync serialization methods?

cc @steveharter @stephentoub

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

---

While adding new tests to System.Text.Json I discovered an issue with the handling of IAsyncEnumerable values in the new sync stream serialization APIs. It would seem like IAsyncEnumerableConverter's sync serialization detection logic is not adequate in identifying the new methods: in debug builds this results in a failed debug assertion whereas release builds end up serializing IAE values successfully, since they accidentally perform sync-over-async enumeration of the source enumerable. We should try to fix this, I can see a couple of options here:

1. Have the converter detect and fail on sync stream serialization methods. Users that rely on the current behavior might regard this as a breaking change.
2. Double down on the existing (accidental) behavior and use sync-over-sync for the sync stream serialization methods. Should we consider allowing the same behavior for non-stream sync serialization methods?

cc @steveharter @stephentoub

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[stephentoub]

Have the converter detect and fail on sync stream serialization methods

Would this be deterministic, i.e. regardless of the async enumerable's contents, whether it's empty or not, whether its operations complete synchronously or not, you'd always get the exception because it'd be based on the shape rather than on the data?

[eiriktsarpalis]

Have the converter detect and fail on sync stream serialization methods

Would this be deterministic, i.e. regardless of the async enumerable's contents, whether it's empty or not, whether its operations complete synchronously or not, you'd always get the exception because it'd be based on the shape rather than on the data?

Good question. For the non-streaming sync serialization methods the converter throws outright (i.e. it's based on the type shape). It's conceivable that we could make it more adaptable but I suspect this might be setting traps for users (e.g. my mocked IAsyncEnumerable responses were passing all unit tests but serialization is crashing when I deploy the app).