Rewrite System.Text.Json stream tests to be async friendly and enable on WASM #38663

akoeplinger · 2020-07-01T12:48:46Z

The tests are using a (De)SerializationWrapper so the same code can be used both for String and Stream types.
It does that by wrapping the async Stream serialization calls in Task.Run().GetAwaiter().GetResult() to turn them into sync calls.
However that doesn't work on WebAssembly since we can't wait on tasks as there's only a single thread.

To fix this inverse the wrapper so the synchronous String calls are turned into async and use normal awaits for the Stream calls.

This allows the test suite to pass on WebAssembly: Tests run: 8349, Errors: 0, Failures: 0, Skipped: 11. Time: 475.528706s

stephentoub · 2020-07-03T00:46:42Z

We can skip fewer now, right? :)

akoeplinger · 2020-07-03T00:47:24Z

Yeah I'm working on it :)

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.cs

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.MultiSegment.cs

… on WASM The tests dealing are using a (De)SerializationWrapper so the same code can be used both for String and Stream types. It does that by wrapping the async Stream serialization calls in `Task.Run().GetAwaiter().GetResult()` to turn them into sync calls. However that doesn't work on WebAssembly since we can't wait on tasks as there's only a single thread. To fix this inverse the wrapper so the synchronous String calls are turned into async and use normal awaits for the Stream calls. This allows the test suite to pass on WebAssembly: `Tests run: 8349, Errors: 0, Failures: 0, Skipped: 11. Time: 475.528706s`

akoeplinger · 2020-07-03T21:34:07Z

@stephentoub this is ready for review now :)

ericstj · 2020-07-08T23:25:20Z

@steveharter @layomia PTAL

stephentoub · 2020-07-09T20:46:57Z

src/libraries/System.Text.Json/tests/JsonDocumentTests.cs

@@ -3650,7 +3647,7 @@ private static string GetCompactJson(TestCaseType testCaseType, string jsonStrin
            return s_compactJson[testCaseType] = existing;
        }

-        [Fact]
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]


It wouldn't be testing the exact same thing, but it seems like this test would "just work" if you changed the Task.WaitAll(tasks) to instead be await Task.WhenAll(tasks) and made the method async, no?

Yeah that does indeed work. I can send a followup PR but I'd like to first get this one in while CI is green :)

Reenabled these tests with your suggestion in the latest iteration.

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Cache.cs

.../System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.AttributePresence.cs

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Cache.cs

.../System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.ParameterMatching.cs

jozkee · 2020-07-10T00:30:15Z

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Stream.cs

-            tasks[12] = Task.Run(async () => await RunTest<Point_With_Dictionary>(Point_With_Dictionary.s_data));
-            tasks[13] = Task.Run(async () => await RunTest<Point_With_Object>(Point_With_Object.s_data));
-
-            Task.WaitAll(tasks);


Can you also apply the pattern suggested in https://github.com/dotnet/runtime/pull/38663/files#r452481795 here?

Isn't the intent of this test to detect race conditions? By awaiting one-by-one doesn't defy said purpose?

It didn't look to me like this test is about race conditions, but I changed it to the Task.WhenAll() pattern nonetheless.

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Stream.cs

src/libraries/System.Text.Json/tests/Serialization/Stream.ReadTests.cs

jozkee · 2020-07-10T00:45:43Z

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.cs

@@ -1609,7 +1610,7 @@ public static void TestPartialJsonReaderSlicesSpecialNumbers(TestCaseType type,
            }
        }

-        [Theory]
+        [ConditionalTheory]


What's the point of a ConditionalTheory without parameters?

This is necessary so throwing SkipTestException works, otherwise the test would fail with an "unexpected" exception.

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Cache.cs

stephentoub

Other than my comments, LGTM.

…json

akoeplinger

@jozkee @stephentoub I addressed your feedback, please take another look :)

akoeplinger · 2020-07-10T10:25:09Z

src/libraries/System.Text.Json/tests/JsonDocumentTests.cs

@@ -3650,7 +3647,7 @@ private static string GetCompactJson(TestCaseType testCaseType, string jsonStrin
            return s_compactJson[testCaseType] = existing;
        }

-        [Fact]
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]


Reenabled these tests with your suggestion in the latest iteration.

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Cache.cs

akoeplinger · 2020-07-10T10:37:51Z

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Stream.cs

-            tasks[12] = Task.Run(async () => await RunTest<Point_With_Dictionary>(Point_With_Dictionary.s_data));
-            tasks[13] = Task.Run(async () => await RunTest<Point_With_Object>(Point_With_Object.s_data));
-
-            Task.WaitAll(tasks);


It didn't look to me like this test is about race conditions, but I changed it to the Task.WhenAll() pattern nonetheless.

src/libraries/System.Text.Json/tests/Serialization/Stream.ReadTests.cs

akoeplinger · 2020-07-10T10:51:00Z

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.cs

@@ -1609,7 +1610,7 @@ public static void TestPartialJsonReaderSlicesSpecialNumbers(TestCaseType type,
            }
        }

-        [Theory]
+        [ConditionalTheory]


This is necessary so throwing SkipTestException works, otherwise the test would fail with an "unexpected" exception.

…json # Conflicts: # src/libraries/tests.proj

stephentoub · 2020-07-10T14:43:14Z

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Stream.cs

+            tasks[0] = Task.Run(async () => await RunTestAsync<Class_With_Ctor_With_65_Params>());
+            tasks[1] = Task.Run(async () => await RunTestAsync<Struct_With_Ctor_With_65_Params>());
+
+            await Task.WhenAll(tasks);


You made these run in parallel just for fun? :)

These were actually already running in parallel before this PR.

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Stream.cs

stephentoub

Other than not being clear on why you added concurrency into some of the tests, LGTM.

steveharter · 2020-07-10T15:08:48Z

src/libraries/System.Text.Json/tests/Serialization/CacheTests.cs

@@ -121,7 +121,7 @@ void Test(int i)
                tasks[i + 1] = Task.Run(() => Test(TestClassCount - 2));
            }

-            Task.WaitAll(tasks);
+            await Task.WhenAll(tasks);


FYI this area of code really does care about threading, and the changes shouldn't affect the multi-threading aspects.

steveharter · 2020-07-10T15:15:51Z

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Cache.cs

            {
-                var obj = Serializer.Deserialize(json, type, options);
+                var obj = await Serializer.DeserializeAsync(json, type, options);


Why was this changed to use the Async version of the API? The original intent was to test non-async APIs for thread safety.

The original test was calling a synchronous method which was then overridden by two different implementations, one that used the underlying sync API and one that used the underlying async API and then blocked waiting on it. This change inverts that, so it's an async API that then uses the underlying sync API and wraps it in a task or the underlying async API. It's done to avoid sync-over-async, which blows up on wasm.

Correct. I've tried to capture this in the PR description but please let me know if I could make this clearer.

steveharter · 2020-07-10T15:21:18Z

src/libraries/System.Text.Json/tests/Serialization/DeserializationWrapper.cs

            {
                if (options == null)
                {
                    options = _optionsWithSmallBuffer;
                }

-                return Task.Run(async () =>
+                using (MemoryStream stream = new MemoryStream(Encoding.UTF8.GetBytes(json)))


So this is the key area of code where we switch the wrapper to use await...

steveharter · 2020-07-10T15:27:38Z

src/libraries/System.Text.Json/tests/Serialization/SerializationWrapper.cs

-                    await JsonSerializer.SerializeAsync(stream, value, inputType, options);
-                    return Encoding.UTF8.GetString(stream.ToArray());
-                }).GetAwaiter().GetResult();
+                using var stream = new MemoryStream();


Again, this is the key area of code where we switch the wrapper to use await without Task.Run.

steveharter · 2020-07-10T15:35:08Z

One global comment is that the renaming of Serialize to SerializeAsync and Deserialize to DeserializeAsync is now a bit confusing since it reads as being async when in the majority of cases it isn't. I realize we can't remove the 'await' modifier but perhaps these forms are more understandable:
await Serializer.Serialize(
await Serializer.SerializeSyncOrAsync(
await Serializer.SerializeWrapper(

akoeplinger · 2020-07-10T16:38:36Z

@steveharter I switched to using Serializer.SerializeWrapper() instead of the Async suffix

Dotnet-GitSync-Bot added the area-System.Text.Json label Jul 1, 2020

marek-safar reviewed Jul 3, 2020

View reviewed changes

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.cs Outdated Show resolved Hide resolved

marek-safar reviewed Jul 3, 2020

View reviewed changes

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.MultiSegment.cs Show resolved Hide resolved

akoeplinger force-pushed the wasm-system-text-json branch from a560a84 to 99192c0 Compare July 3, 2020 21:31

akoeplinger changed the title ~~WASM: enable System.Text.Json tests~~ Rewrite System.Text.Json stream tests to be async friendly and enable on WASM Jul 3, 2020

akoeplinger marked this pull request as ready for review July 3, 2020 21:33

akoeplinger requested review from jozkee, layomia and steveharter as code owners July 3, 2020 21:33

akoeplinger added 2 commits July 7, 2020 16:37

Merge branch 'master' into wasm-system-text-json

e36fa5a

Merge branch 'master' into wasm-system-text-json

42ab5a8

steveisok mentioned this pull request Jul 8, 2020

[Wasm] Tracking active test failures #38422

Closed

99 tasks

stephentoub reviewed Jul 9, 2020

View reviewed changes

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Cache.cs Outdated Show resolved Hide resolved

jozkee reviewed Jul 10, 2020

View reviewed changes

stephentoub reviewed Jul 10, 2020

View reviewed changes

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Cache.cs Outdated Show resolved Hide resolved

stephentoub approved these changes Jul 10, 2020

View reviewed changes

akoeplinger added 2 commits July 10, 2020 12:20

Merge remote-tracking branch 'upstream/master' into wasm-system-text-…

e44acd3

…json

PR feedback

e9104ea

akoeplinger commented Jul 10, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into wasm-system-text-…

85f16ab

…json # Conflicts: # src/libraries/tests.proj

stephentoub reviewed Jul 10, 2020

View reviewed changes

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Stream.cs Show resolved Hide resolved

stephentoub reviewed Jul 10, 2020

View reviewed changes

src/libraries/System.Text.Json/tests/Serialization/ConstructorTests/ConstructorTests.Stream.cs Show resolved Hide resolved

stephentoub approved these changes Jul 10, 2020

View reviewed changes

steveharter reviewed Jul 10, 2020

View reviewed changes

akoeplinger added 2 commits July 10, 2020 18:25

Disable a test that shouldn't be running on Mono interpreter

d892de5

PR feedback: use Serializer.SerializeWrapper() instead of Async suffix

43f95a9

steveharter approved these changes Jul 10, 2020

View reviewed changes

akoeplinger merged commit 95e3dcc into dotnet:master Jul 10, 2020

akoeplinger deleted the wasm-system-text-json branch July 10, 2020 20:43

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite System.Text.Json stream tests to be async friendly and enable on WASM #38663

Rewrite System.Text.Json stream tests to be async friendly and enable on WASM #38663

akoeplinger commented Jul 1, 2020 •

edited

Loading

stephentoub commented Jul 3, 2020

akoeplinger commented Jul 3, 2020

akoeplinger commented Jul 3, 2020

ericstj commented Jul 8, 2020

stephentoub Jul 9, 2020

akoeplinger Jul 9, 2020

akoeplinger Jul 10, 2020

jozkee Jul 10, 2020

jozkee Jul 10, 2020

akoeplinger Jul 10, 2020

jozkee Jul 10, 2020

akoeplinger Jul 10, 2020

stephentoub left a comment

akoeplinger left a comment

akoeplinger Jul 10, 2020

akoeplinger Jul 10, 2020

akoeplinger Jul 10, 2020

stephentoub Jul 10, 2020

akoeplinger Jul 10, 2020

stephentoub left a comment

steveharter Jul 10, 2020

steveharter Jul 10, 2020

stephentoub Jul 10, 2020

akoeplinger Jul 10, 2020

steveharter Jul 10, 2020

steveharter Jul 10, 2020

steveharter commented Jul 10, 2020

akoeplinger commented Jul 10, 2020

Rewrite System.Text.Json stream tests to be async friendly and enable on WASM #38663

Rewrite System.Text.Json stream tests to be async friendly and enable on WASM #38663

Conversation

akoeplinger commented Jul 1, 2020 • edited Loading

stephentoub commented Jul 3, 2020

akoeplinger commented Jul 3, 2020

akoeplinger commented Jul 3, 2020

ericstj commented Jul 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

akoeplinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveharter commented Jul 10, 2020

akoeplinger commented Jul 10, 2020

akoeplinger commented Jul 1, 2020 •

edited

Loading