Update M.E.DependencyModel to use S.T.Json source for netstandard2.0 #5009

ahsonkhan · 2019-01-19T14:15:24Z

Partially addresses https://github.com/dotnet/core-setup/issues/3311

Initial attempt to port Microsoft.Extensions.DependencyModel to use the System.Text.Json source package for TFM netstandard2.0. Note: ~~There are some API changes going through so this is still WIP.~~ We have the JsonReader and JsonWriter available atm so only using the components that are ready.

The System.Text.Json binary is in-box on .NET Core 3.0.

We can't remove the reference to Newtonsoft since M.E.DependencyModel has multiply TFMs: net451;netstandard1.3;netstandard1.6;netstandard2.0

Is it possible to drop support for all but netstandard2.0? Otherwise, we have to conditionally include the source package.

See https://github.com/dotnet/corefx/blob/master/src/System.Text.Json/source_package/README.md

Here are the diffs to try to highlight what changed during the port:

DependencyContextJsonReader.cs: https://www.diffchecker.com/KEX0UhJd
DependencyContextWriter.cs: https://www.diffchecker.com/GByfr9Gi
JsonTextReaderExtensions.cs: https://www.diffchecker.com/7NfdaYUn

Related PR: #5010

cc @joshfree, @bartonjs, @steveharter, @eerhardt, @KrzysztofCwalina, @JamesNK, @glennc

ahsonkhan · 2019-01-19T14:16:25Z

src/managed/Microsoft.Extensions.DependencyModel/StreamBufferWriter.Netstandard2_0.cs

+
+namespace Microsoft.Extensions.DependencyModel
+{
+    internal struct StreamBufferWriter : IBufferWriter<byte>, IDisposable


We should consider adding such an adapter between IBufferWriter and Stream, publicly.

cc @stephentoub, @davidfowl

Yes please. We have something similar in SignalR https://github.com/aspnet/AspNetCore/blob/958e9049894137bb08c0d239dca0cda7c850a691/src/SignalR/common/Shared/MemoryBufferWriter.cs#L14 (ignore the fact that it's also a Stream, we just did that for other reasons).

Also the PipeWriter implemenation over a Stream is 90% similar https://github.com/aspnet/AspNetCore/blob/958e9049894137bb08c0d239dca0cda7c850a691/src/Http/Http/src/StreamPipeWriter.cs.

This is something that @JeremyKuhne might have looked into.
/cc @danmosemsft

https://github.com/dotnet/corefx/issues/34894

ahsonkhan · 2019-01-19T14:16:39Z

src/managed/Microsoft.Extensions.DependencyModel/Microsoft.Extensions.DependencyModel.csproj

@@ -6,29 +6,41 @@
    <TargetFrameworks>net451;netstandard1.3;netstandard1.6;netstandard2.0</TargetFrameworks>


What should we do about the other TFMs?

ahsonkhan · 2019-01-19T14:17:30Z

src/managed/Microsoft.Extensions.DependencyModel/JsonTextReaderExtensions.Netstandard2_0.cs

+            return false;
+        }
+
+        internal static void Skip(this ref Utf8JsonReader jsonReader)


There is an issue to add this API: https://github.com/dotnet/corefx/issues/33295

ahsonkhan · 2019-01-19T14:18:34Z

src/managed/Microsoft.Extensions.DependencyModel/DependencyContextWriter.Netstandard2_0.cs

+
+                // Add runtime-specific assets
+                bool wroteObjectStart = false;
+                wroteObjectStart = AddRuntimeSpecificAssetGroups(DependencyContextStrings.RuntimeAssetType, runtimeLibrary.RuntimeAssemblyGroups, wroteObjectStart, ref jsonWriter);


This was necessary since the writer is forward only (while building a JObject and writing at the end did not care about the order).

probably worth a comment in the code then until this moves to the JsonDocument (writer)

I don't think we would move this to JsonDocument (as it is written today) especially since this is writing (not reading). I was only suggesting that we needed this bool wroteObjectStart due to the ordering of the writes being important (while the ordering of adding to JObject is not really important).

ahsonkhan · 2019-01-19T14:21:23Z

src/managed/Microsoft.Extensions.DependencyModel/DependencyContextJsonReader.Netstandard2_0.cs

+
+        private const int UnseekableStreamInitialRentSize = 4096;
+
+        private static ArraySegment<byte> ReadToEnd(Stream stream)


Borrowed from https://github.com/dotnet/corefx/blob/d1da417dbf38079f06b397bdf3c8f51b94e62238/src/System.Text.Json/src/System/Text/Json/JsonDocument.Parse.cs#L182. I am working on providing better stream support with the reader, but it still results in the calling code being overly complex. Reading the stream to the end atm for simplicity.

src/managed/Microsoft.Extensions.DependencyModel/DependencyContextJsonReader.Netstandard2_0.cs

ahsonkhan · 2019-01-19T15:06:32Z

src/managed/Microsoft.Extensions.DependencyModel/JsonTextReaderExtensions.Netstandard2_0.cs

+        internal static Exception CreateUnexpectedException(ref Utf8JsonReader reader, string expected)
+        {
+            return new FormatException($"Unexpected character encountered, excepted '{expected}' " +
+                                       $"at line {reader.CurrentState._lineNumber} position {reader.CurrentState._bytePositionInLine}");


The Utf8JsonReader doesn't track/create a path.

IMO you should only be using public APIs from the reader.

If position info isn't already public then this is a good argument for providing it. If you need it here then other users of the reader will also likely use it.

IMO you should only be using public APIs from the reader.

Agreed.

However, I don't really understand the need for this method. Why not rely on the exceptions thrown by the JsonReader itself? But, you are right that we may want to expose these publicly.

cc @eerhardt

Looks like the code was that way since it was migrated from using JObject to simply the JsonTextReader.

dotnet/cli@a02d002

I assume it is so we can give "good" exception messages about exactly where we expect a new object/array/etc. What is the exception message you get when you rely on the exceptions thrown by the JsonReader itself?

You get a JsonReaderException with LineNumber and BytePosition included in the message, when reading an invalid JSON.

The message string is generally one of the following:
https://github.com/dotnet/corefx/blob/2d9dd533e16889ea4d279162df4039982bfbbca2/src/System.Text.Json/src/Resources/Strings.resx#L120-L284
See where we throw them:
https://github.com/dotnet/corefx/blob/2d9dd533e16889ea4d279162df4039982bfbbca2/src/System.Text.Json/src/System/Text/Json/ThrowHelper.cs#L231

For example, for invalid JSON, you could see a message like: Expected end of string, but instead reached end of data.
OR as another example:
Message: System.Text.Json.JsonReaderException : '}' is an invalid start of a property name. Expected a '"'. LineNumber: 7 | BytePositionInLine: 4.

Now, assuming that the JSON itself is valid, and we are trying to validate the schema, calling GetString on something other than json string would throw an invalid operation exception with an invalid cast message: Cannot get the value of a token type '{0}' as a {1}.
https://github.com/dotnet/corefx/blob/2d9dd533e16889ea4d279162df4039982bfbbca2/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs#L26

This exception message wouldn't contain the info about where we expect a new object/array.

If position info isn't already public then this is a good argument for providing it. If you need it here then other users of the reader will also likely use it.

https://github.com/dotnet/corefx/issues/34768

src/managed/Microsoft.Extensions.DependencyModel/DependencyContextJsonReader.Netstandard2_0.cs

bartonjs · 2019-01-19T18:37:16Z

src/managed/Microsoft.Extensions.DependencyModel/DependencyContextJsonReader.Netstandard2_0.cs

+
+            if (runtimeTarget == null)
+            {
+                throw new FormatException("No runtime target found");


I'm rather surprised that core-setup isn't using resource strings

src/managed/Microsoft.Extensions.DependencyModel/DependencyContextJsonReader.Netstandard2_0.cs

eerhardt · 2019-01-19T20:04:15Z

src/managed/Microsoft.Extensions.DependencyModel/JsonTextReaderExtensions.Netstandard2_0.cs

+
+namespace Microsoft.Extensions.DependencyModel
+{
+    internal static class JsonTextReaderExtensions


It feels like all these methods are things that most consumers would want... and it is obviously a non-trivial amount of code.

@eerhardt A lot of the code in these extension methods is just about changing the exception type, and combining the move and get operations. If InvalidOperationException were acceptable then ReadAsString() is just

reader.Next(); return reader.GetStringValue();

The return value of Next is important when using the reader in async/chunked mode; and an exception model is what it is...

src/managed/Microsoft.Extensions.DependencyModel/DependencyContextWriter.Netstandard2_0.cs

src/managed/Microsoft.Extensions.DependencyModel/Microsoft.Extensions.DependencyModel.csproj

src/managed/Microsoft.Extensions.DependencyModel/StreamBufferWriter.Netstandard2_0.cs

to RFC.

trailing commas.

behavior.

src/managed/Microsoft.Extensions.DependencyModel/ArrayBufferWriter.cs

src/managed/Microsoft.Extensions.DependencyModel/Microsoft.Extensions.DependencyModel.csproj

eerhardt · 2019-01-24T21:33:42Z

src/test/Microsoft.Extensions.DependencyModel.Tests/DependencyContextJsonReaderTest.cs

@@ -30,7 +28,7 @@ public void ReadsRuntimeTargetInfo()
        ""signature"":""target-signature""
    },
    ""targets"": {
-        "".NETCoreApp,Version=v1.0/osx.10.10-x64"": {},
+        "".NETCoreApp,Version=v1.0/osx.10.10-x64"": {}


What's really weird is that using DependencyModel v2.1, I tried adding trailing commas and I was getting exceptions being thrown from the current implementation. I'm surprised that these tests are passing.

src/managed/Microsoft.Extensions.DependencyModel/ArrayBufferWriter.cs

src/managed/Microsoft.Extensions.DependencyModel/DependencyContextJsonReader.Utf8JsonReader.cs

src/managed/Microsoft.Extensions.DependencyModel/UnifiedJsonReader.Utf8JsonReader.cs

using UnifiedJsonWriter extract out common code by using UnifiedJsonWriterr

ahsonkhan · 2019-01-28T11:18:53Z

Build failures for Arm/Arm64 on Windows are unrelated: https://dev.azure.com/dnceng/public/_build/results?buildId=81361

2019-01-28T10:44:06.7388911Z   Calling "D:\a\1\s\src\corehost\Windows\gen-buildsys-win.bat D:\a\1\s\ "15 2017" arm b41cae7e8c4dee8b3e2bda9fec2f17b5d29c5fbf 3.0.0-preview-27328-2 3.0.0-preview-27328-2 3.0.0-preview-27328-2 3.0.0-preview-27328-2"
2019-01-28T10:44:06.7389019Z   "Computed RID for native build is win-arm"
2019-01-28T10:44:06.7389139Z   "C:\Program Files\CMake\bin\cmake.exe" D:\a\1\s\src\corehost\Windows\.. "-DCMAKE_SYSTEM_VERSION=10.0" "-DCLI_CMAKE_RUNTIME_ID:STRING=win-arm" "-DCLI_CMAKE_HOST_VER:STRING=3.0.0-preview-27328-2" "-DCLI_CMAKE_APPHOST_VER:STRING=3.0.0-preview-27328-2" "-DCLI_CMAKE_HOST_FXR_VER:STRING=3.0.0-preview-27328-2" "-DCLI_CMAKE_HOST_POLICY_VER:STRING=3.0.0-preview-27328-2" "-DCLI_CMAKE_PKG_RID:STRING=win-arm" "-DCLI_CMAKE_COMMIT_HASH:STRING=b41cae7e8c4dee8b3e2bda9fec2f17b5d29c5fbf" "-DCLI_CMAKE_PLATFORM_ARCH_ARM=1" "-DCMAKE_INSTALL_PREFIX=D:/a/1/s/src/corehost/../../bin/win-arm.Debug/corehost" "-DCLI_CMAKE_RESOURCE_DIR:STRING=D:\a\1\s\src\corehost\..\..\bin\obj\win-arm.Debug\hostResourceFiles" -G "Visual Studio 15 2017 ARM" 
2019-01-28T10:44:11.3988591Z   -- Selecting Windows SDK version 10.0.17763.0 to target Windows 10.0.
2019-01-28T10:44:25.0285854Z   -- The C compiler identification is unknown
2019-01-28T10:44:29.2688030Z   -- The CXX compiler identification is unknown
2019-01-28T10:44:29.3789095Z   CMake Error in CMakeLists.txt:
2019-01-28T10:44:29.3789556Z     No CMAKE_C_COMPILER could be found.
2019-01-28T10:44:29.3789731Z   
2019-01-28T10:44:29.3789913Z   
2019-01-28T10:44:29.3790220Z   
2019-01-28T10:44:29.3790372Z   CMake Error in CMakeLists.txt:
2019-01-28T10:44:29.3790550Z     No CMAKE_CXX_COMPILER could be found.
2019-01-28T10:44:29.3790697Z   
2019-01-28T10:44:29.3790843Z   
2019-01-28T10:44:29.3791011Z   
2019-01-28T10:44:29.3900213Z   -- Configuring incomplete, errors occurred!
2019-01-28T10:44:29.5281405Z   See also "D:/a/1/s/bin/obj/win-arm.Debug/corehost/CMakeFiles/CMakeOutput.log".
2019-01-28T10:44:29.5290897Z   See also "D:/a/1/s/bin/obj/win-arm.Debug/corehost/CMakeFiles/CMakeError.log".
2019-01-28T10:44:29.5359326Z   Failed to generate native component build project!
2019-01-28T10:44:29.5359586Z D:\a\1\s\src\corehost\build.proj(79,5): error MSB3073: The command "D:\a\1\s\src\corehost\build.cmd Debug arm apphostver 3.0.0-preview-27328-2 hostver 3.0.0-preview-27328-2 fxrver 3.0.0-preview-27328-2 policyver 3.0.0-preview-27328-2 commit b41cae7e8c4dee8b3e2bda9fec2f17b5d29c5fbf rid win-arm portable" exited with code 1.
2019-01-28T10:44:29.5507299Z 
2019-01-28T10:44:29.5614879Z Build FAILED.
2019-01-28T10:44:29.5628283Z 
2019-01-28T10:44:29.5633185Z D:\a\1\s\src\corehost\build.proj(79,5): error MSB3073: The command "D:\a\1\s\src\corehost\build.cmd Debug arm apphostver 3.0.0-preview-27328-2 hostver 3.0.0-preview-27328-2 fxrver 3.0.0-preview-27328-2 policyver 3.0.0-preview-27328-2 commit b41cae7e8c4dee8b3e2bda9fec2f17b5d29c5fbf rid win-arm portable" exited with code 1.
2019-01-28T10:44:29.5638856Z     0 Warning(s)
2019-01-28T10:44:29.5642108Z     1 Error(s)

cc @dotnet/dnceng

JsonReaderException.

MattGal · 2019-01-28T18:09:39Z

@ahsonkhan I looked into this in https://github.com/dotnet/core-setup/issues/5052, see notes there.

eerhardt · 2019-01-28T19:59:32Z

src/managed/Microsoft.Extensions.DependencyModel/Microsoft.Extensions.DependencyModel.csproj

+        <PackageReference Include="Microsoft.Bcl.Json.Sources" Version="4.6.0-preview.19072.2">
+          <PrivateAssets>All</PrivateAssets>
+        </PackageReference>
+        <PackageReference Include="System.Memory" Version="4.5.2" />


Do we really need 4.5.2?

As opposed to what? 4.5.2 is the latest version available.

Right, but is the latest version the right thing to choose? Typically libraries don't update their dependencies just because there is a new version out. Doing so causes anyone who references you to use the latest version, which may not be the right thing.

Is there a specific reason you chose 4.5.2? or was it just because it was the latest version?

In reply to: 251573648 [](ancestors = 251573648)

Typically libraries don't update their dependencies just because there is a new version out. Doing so causes anyone who references you to use the latest version, which may not be the right thing.

Good point. I didn't have a specific need for the latest. I believe I can safely downgrade to 4.5.0.

src/managed/Microsoft.Extensions.DependencyModel/Microsoft.Extensions.DependencyModel.csproj

src/test/Microsoft.Extensions.DependencyModel.Tests/DependencyContextJsonReaderTest.cs

…eInboxJson

…otnet/core-setup#5009) * Update M.E.DependencyModel to use in-box S.T.Json for netstandard2.0 * Move StreamBufferWriter to a separate file. * Return array back to the pool on success. * Update test json and remove trailing commas to make it valid according to RFC. * Fix off-by-one error in skip, add a null string check, and remove other trailing commas. * Write end of object before flushing. * Add missing write end array to close the array block. * Update to latest source package with some API changes. * Address PR feedback. * Split DepContextJsonReader into common, newtonsoft, and netstandard. * Try to minimize the diff between Newtonsoft and S.T.Json. * Remove the .Common prefix for the source that is shared between the impls. * Explicitly add access modifiers and some code formatting fix. * Add debug asserts and add comment to where ReadToEnd comes from. * Mark methods as static where possible. * Update code formatting (use var, inline outs, etc.) * Use implicit cast to span and use try/finally block instead. * Always check to resize in case stream.Length changes. * Use an array-based buffer writer rather than one that supports stream. * Add a unified json reader to reduce code duplication. * Remove JsonTextReaderExtensions and cleanup some changes to keep the diff cleaner. * Skip comments by default. * Allow comments on the reader but throw FormatException to mimic current behavior. * Adjust the exception message comparion assert. * Use the latest LangVersion (7.3) required by the source package. * Rename partial files using Utf8JsonReader/JsonTextReader suffixes. * Address PR feedback - cleanup csproj and remove unused APIs. * Use JsonTextWriter (instead of JObject) and extract out common code by using UnifiedJsonWriter extract out common code by using UnifiedJsonWriterr * Rename Write to WriteCore to match the Read * Explicitly disallow comments and update test to assert JsonReaderException. * Add source package version to Versions.props. Commit migrated from dotnet/core-setup@45f9401

ahsonkhan added 2 commits January 19, 2019 01:18

Update M.E.DependencyModel to use in-box S.T.Json for netstandard2.0

744a6d8

Move StreamBufferWriter to a separate file.

f5d1518

ahsonkhan commented Jan 19, 2019

View reviewed changes

Return array back to the pool on success.

a376775

ahsonkhan mentioned this pull request Jan 19, 2019

[WIP] Update core-setup local tools to use S.T.Json for netstandard2.0 instead #5010

Closed

ahsonkhan commented Jan 19, 2019

View reviewed changes