Support for .NET Standard 2.0 and 2.1 #513

mtmk · 2024-06-10T19:34:15Z

Main focus of this PR is to introduce .NET Standard support as a preview feature and make absolutely sure the .NET 6 & 8 support is not affected at all.

To reduce the performance impact on platforms that support 2.1 (e.g. Unity) we take advantage of 2.1 APIs where possible. We can create follow up PRs to improve .NET Standard performance for both 2.0 and 2.1.

InboxSub WeakRef changes for 2.0

There are minor changes for 2.1 but 2.0 seems to lack quite a few APIs so changes in this class might require a little more scrutiny.

TLS Support

Most of the APIs aren't available in netstandard, hence TLS support will be limited in targets using netstandard. However the important thing is to make sure net8.0 isn't affected at all. Suggestion is to improve netstandard TLS support in follow up PRs and have a limited support for TLS features at least in the initial preview release.

Code Changes in net6.0 and net8.0 Targets

There are minor code changes which will affect the net6.0 and net8.0 builds. These code changes reduce preprocessor usage significantly, however they shouldn't introducing any functional changes or side-effects.

ValueTask.CompletedTask doesn't exist in netstandard so instead we changed all our usages to default as it's implemented in source for ValueTask.CompletedTask

- return ValueTask.CompletedTask;
+ return default;

ValueTask.FromResult doesn't exist in netstandard so instead we changed all our usages to using .ctor as it's implemented in source for ValueTask.FromResult

- return ValueTask.FromResult(group);
+ return new ValueTask<Group>(group);

Removed unused classes CancellationTimerPool, FixedArrayBufferWriter and ThreadPoolWorkItem and their usages.
Replaced TimeSpan operators with Ticks calculation similar to what happens in current TimeSpan source except some bounds checks which should not affect us since we're using constants.

- _backoff *= 2;
+ _backoff = new TimeSpan(_backoff.Ticks * 2);

Case-insensitive string comparison for error message checks:

- Error = error;
- IsAuthError = Error.Contains("authorization violation", StringComparison.OrdinalIgnoreCase)
+ Error = error.ToLower();
+ IsAuthError = Error.Contains("authorization violation")

In string.Split(), use char instead of string in ReplyToDateTimeAndSeq.cs

- var originalTokens = reply.Split(".").AsSpan();
+ var originalTokens = reply.Split('.').AsSpan();

Also in NatsKVStore.cs

- if (key.StartsWith('.') || key.EndsWith('.'))
+ if (key.StartsWith(".") || key.EndsWith("."))

Tests

There are no changes to the existing tests to make sure no regressions happen. Only couple of tests are deleted related to now unused classes in the library.

The new 'Platform' test targets .NET Framework 4.8.1 and uses netstandard2.0 version of the library. These tests broadly covers all the libraries demonstrating that netstandard target works as expected at least for the basic functionality.

Idea is eventually merge all the tests and run them for all the targets for best platform coverage.

Info

https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0#select-net-standard-version

resolves Support .NET Standard 2.0 #487

…ard-support

caleblloyd · 2024-06-14T12:35:05Z

Should we put this in a single file? So we don't have to put it all over the place?

#if NETSTANDARD
global using NATS.Client.Core.Internal.NetStandardExtensions;
#endif

caleblloyd · 2024-06-14T12:49:07Z

src/NATS.Client.Core/Internal/SslStreamConnection.cs

            using var cts = new CancellationTokenSource(timeout);
            await _sslStream.AuthenticateAsClientAsync(options, cts.Token).ConfigureAwait(false);
+#else
+            await _sslStream.AuthenticateAsClientAsync(uri.Host).ConfigureAwait(false);


It looks like this effectively ignores _tlsOpts on netstandard

I think the problem was I couldn't see na easy way of loading the PEM files

Yea we may need to bring in Bouncy Castle or something to do it. Not sure if someone has made a good compat library

created Improve netstandard TLS support #520

src/NATS.Client.Core/Commands/CommandWriter.cs

to11mtm

Added some food for thought but overall this is awesome. <3

to11mtm · 2024-06-17T00:28:32Z

src/NATS.Client.Core/Internal/netstandard.cs

+        internal static bool IsNotCompletedSuccessfully(this Task? task)
+        {
+            return task != null && (!task.IsCompleted || task.IsCanceled || task.IsFaulted);
+        }
+    }


Not sure whether it's actually better to bang against Task.Status and/or Task.IsCompleted+Task.Exception!=null or not, here's some sharplab as FTT

done (task.Status == TaskStatus.RanToCompletion)

to11mtm · 2024-06-17T00:30:13Z

src/NATS.Client.Core/Commands/CommandWriter.cs

+#if NETSTANDARD2_0
+        if (_flushTask.IsNotCompletedSuccessfully())
+#else
        if (_flushTask is { IsCompletedSuccessfully: false })
+#endif


Stupid question: would it be cleaner to put the logic shifts in IsNotCompletedSuccessfully() extension method and just aggressive inline? Or would it be too ugly for how shims should be composed?

yes good point, fewer #if the better!

to11mtm · 2024-06-17T00:32:19Z

src/NATS.Client.Core/INatsSerialize.cs

+#if NETSTANDARD2_0
+        var span = buffer.IsSingleSegment ? buffer.First.Span : buffer.ToArray();
+#else
        var span = buffer.IsSingleSegment ? buffer.FirstSpan : buffer.ToArray();
+#endif


Another potential candidate for ext-method-consolidation is buffer.FirstSpan vs buffer.First.Span.

I should note that, AFAIR, if the extension method is internal, Resharper/Rider can always let team 're-dupe' the callsites later easily if ppl change their minds.

makes sense

to11mtm · 2024-06-17T00:40:25Z

src/NATS.Client.Core/Internal/NuidWriter.cs

+#if NETSTANDARD2_0
+            return new string(buffer.ToArray());
+#else


IIRC at least if we are pulling in System.Memory for netstandard we can just do buffer.ToString() here and there's logic to handle that for ReadOnlySpan<char> without having to do the unsafe nasty on your own :)

very good observation. yes, there is an exception for span in ToString(). nice!

to11mtm · 2024-06-17T00:41:40Z

src/NATS.Client.Core/Internal/NuidWriter.cs

+#if NETSTANDARD2_0
+        var randomBytes = new byte[(int)PrefixLength];
+
+        if (rng == null)
+        {
+            using var randomNumberGenerator = RandomNumberGenerator.Create();
+            randomNumberGenerator.GetBytes(randomBytes);
+        }
+#else


Are we able to explain why the randomness this will do is as safe as newer platforms? (It's been a while since I've used the RNG in standard but figured it was good to ask)

I think as long as we use 'some kind of' RNG we should be fine here. This is about generating unique IDs and there shouldn't be a huge security concern but please feel free to challenge that, I might be missing something.

Remarks from netstandard System.Security.Cryptography.RandomNumberGenerator class docs:

Cryptographic random number generators create cryptographically strong random values.

Using the static members of this class is the preferred way to generate random values.

To create a random number generator, call the Create() method. This is preferred over calling the constructor of the derived class RNGCryptoServiceProvider, which is not available on all platforms.

to11mtm · 2024-06-17T00:46:30Z

src/NATS.Client.Core/Internal/SubscriptionManager.cs

+#if NETSTANDARD2_0
+            _bySid.TryRemove(subMetadata.Sid, out _);
+#else
            _bySid.Remove(subMetadata.Sid, out _);
+#endif


Is there value in using the different overloads here?

to11mtm · 2024-06-17T00:49:17Z

src/NATS.Client.Core/Internal/ThreadPoolWorkItem.cs

-
-namespace NATS.Client.Core.Internal;
-
-internal sealed class ThreadPoolWorkItem<T> : IThreadPoolWorkItem


it was a good one. did you write it back in the day?

to11mtm · 2024-06-17T00:50:18Z

src/NATS.Client.Core/NatsConnection.RequestReply.cs

@@ -128,7 +132,11 @@ internal static string NewInbox(ReadOnlySpan<char> prefix)
            var remaining = buffer.Slice((int)totalPrefixLength);
            var didWrite = NuidWriter.TryWriteNuid(remaining);
            Debug.Assert(didWrite, "didWrite");
+#if NETSTANDARD2_0
+            return new string(buffer.ToArray());


Another potential case for buffer.ToString() instead

to11mtm · 2024-06-17T01:07:27Z

src/NATS.Client.KeyValueStore/NatsKVStore.cs

+                var bytes = Convert.FromBase64String(response.Message.Data);
+                var buffer = new ReadOnlySequence<byte>(bytes);
+                try
+                {
+                    data = serializer.Deserialize(buffer);
+                }
+                catch (Exception e)
+                {
+                    deserializeException = new NatsDeserializeException(buffer.ToArray(), e);
+                    data = default;
+                }


Couple of notes here:

I think if we can, we should try to keep behavior consistent, even if that means the Convert.FromBase64String is done as a try-catch.

Is there an issue on the backlog to convert the StreamMessage.Data type into something a bit less thrashy? System.Buffers.Text has To/From for utf8 byte sequences and it looks like they work with netstandard20, adapting to such an approach longer term may help consolidate this code as well as lower the deserialization cost of KV and other stream reads in NET8 (as we can hold the base64 in UTF8 bytes rather than UTF16 string, maybe pool in future too? Not sure how easy it would be to do that part though...)

(1) 👍
(2) that sounds good. created one: #518

to11mtm · 2024-06-17T01:16:12Z

src/NATS.Client.ObjectStore/Internal/Encoder.cs

+#if NETSTANDARD2_0
+            var hash = sha256.ComputeHash(value.ToArray());
+            hash.AsSpan().CopyTo(destination);
+#else


Where this gets tricky is, if the object is above a certain size, grab a pooled array and copy the contents over.

-OR-, it might be better to pay the small price of something like CommunityToolkit.HighPerformance's ReadOnlyMemory<byte> stream adapter to avoid the copies.

created #519

* Support for .NET Standard 2.0 and 2.1 (#513)

caleblloyd

LGTM

* Support for .NET Standard 2.0 and 2.1 (#513)

mtmk added 26 commits June 10, 2024 20:32

Support for .NET Standard 2.0 and 2.1

850348c

Merge remote-tracking branch 'refs/remotes/origin/main' into netstand…

3b7c839

…ard-support

merge fix

c4c1994

Auth error fix

b1d53c9

dotnet format

b190aae

dotnet format

1560f40

standardize test utils

dca0c0e

standardize JetStream

c61d0c3

dotnet format

117fe37

standardize KV

fcdec62

standardize JetStream tweaks

4c8cae6

standardize object store

c501259

standardize services

76c3bc7

standardize extensions

42e8255

fixed code for failing tests

91ffcda

dotnet format

2013c0a

framework core tests

99c05c8

test fix

055d693

other tests

abc6b02

revert net481 tests

f6b55ee

use read loop

3168197

tidy-up

123514f

platform tests

4dbcd84

windows test tweak

b206685

dotnet format

83f02c5

platform basic tests

5516b20

mtmk requested a review from caleblloyd June 14, 2024 01:03

mtmk marked this pull request as ready for review June 14, 2024 01:03

mtmk self-assigned this Jun 14, 2024

revert unnecessary changes to test utils

2cee902

simplify

6dd35d6

mtmk added 2 commits June 14, 2024 13:36

use Nullable package

0ce7c80

simplify

cf213e6

mtmk requested a review from scottf June 14, 2024 12:43

caleblloyd reviewed Jun 14, 2024

View reviewed changes

mtmk added 3 commits June 14, 2024 13:52

simplify diffs

1d2166a

global using

e0c282f

use Nullable package for DoesNotReturn

2ea8828

mtmk commented Jun 14, 2024

View reviewed changes

src/NATS.Client.Core/Commands/CommandWriter.cs Outdated Show resolved Hide resolved

mtmk added 2 commits June 14, 2024 14:41

dotnet format

4b5a94c

use NETSTANDARD in defs

c56cc54

to11mtm reviewed Jun 17, 2024

View reviewed changes

mtmk added 5 commits June 17, 2024 11:32

IsNotCompletedSuccessfully extension

7bf683b

first span extension

74b3b9c

use span-char toString

9695a1d

KV base64 exception handling

695bc6e

dotnet format

bc2aadc

This was referenced Jun 18, 2024

StoredMessage.Data as System.Buffers.Text #518

Closed

Improve netstandard SHA performance #519

Open

Improve netstandard TLS support #520

Closed

mtmk added 3 commits June 18, 2024 15:35

guarding MemoryMarshal.TryGetArray

ebce217

adjusted dictionary remove call

6641414

Merge branch 'refs/heads/main' into netstandard-support

fb24f7f

mtmk added a commit that referenced this pull request Jun 20, 2024

Release 2.3.0-preview.1

8edf506

* Support for .NET Standard 2.0 and 2.1 (#513)

mtmk mentioned this pull request Jun 20, 2024

Release 2.3.0-preview.1 #522

Merged

caleblloyd approved these changes Jun 20, 2024

View reviewed changes

mtmk merged commit a7c88e1 into main Jun 20, 2024
10 checks passed

mtmk deleted the netstandard-support branch June 20, 2024 13:16

mtmk added a commit that referenced this pull request Jun 24, 2024

Release 2.3.0-preview.1 (#522)

06ec115

* Support for .NET Standard 2.0 and 2.1 (#513)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for .NET Standard 2.0 and 2.1 #513

Support for .NET Standard 2.0 and 2.1 #513

mtmk commented Jun 10, 2024 •

edited

Loading

caleblloyd commented Jun 14, 2024 •

edited

Loading

caleblloyd Jun 14, 2024

mtmk Jun 14, 2024

caleblloyd Jun 14, 2024

mtmk Jun 18, 2024 •

edited

Loading

to11mtm left a comment

to11mtm Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 18, 2024

to11mtm Jun 17, 2024

mtmk Jun 18, 2024

caleblloyd left a comment


		namespace NATS.Client.Core.Internal;

		internal sealed class ThreadPoolWorkItem<T> : IThreadPoolWorkItem

Support for .NET Standard 2.0 and 2.1 #513

Support for .NET Standard 2.0 and 2.1 #513

Conversation

mtmk commented Jun 10, 2024 • edited Loading

InboxSub WeakRef changes for 2.0

TLS Support

Code Changes in net6.0 and net8.0 Targets

Tests

Info

caleblloyd commented Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtmk Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

to11mtm 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleblloyd left a comment

Choose a reason for hiding this comment

mtmk commented Jun 10, 2024 •

edited

Loading

caleblloyd commented Jun 14, 2024 •

edited

Loading

mtmk Jun 18, 2024 •

edited

Loading