Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/StreamJsonRpc/CommonMethodNameTransforms.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Text.Json;

namespace StreamJsonRpc;

/// <summary>
Expand All @@ -23,7 +25,7 @@ public static Func<string, string> CamelCase
throw new ArgumentNullException();
}

return Utilities.ToCamelCase(name);
return JsonNamingPolicy.CamelCase.ConvertName(name);
Copy link
Member Author

Choose a reason for hiding this comment

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

This one isn't 100% identical. The System.Text.Json method is almost exactly the same as the ToCamelCase utility being replaced, except that the STJ method looks for ' ' as a separator whereas the ToCamelCase routine looks for anything that's char.IsSeparator. If that difference is important, this part can be reverted.

Copy link
Member

Choose a reason for hiding this comment

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

The separator is of no concern since we're translating .NET identifiers, which have no whitespace.
What is possibly an issue however is that you're adding an STJ dependency in code that runs in non-STJ scenarios, requiring STJ to be loaded where it may not have been before. Visual Studio counts assembly loads, so we deliberately avoid dependencies on particular serialization libraries unless we're in code that already is specific to that library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, so it's in the transitive graph here but you're able to avoid loading it by avoiding it in some code paths? How are regressions caught other than by your watchful code review eye? After the fact by the perf tooling?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if nothing else than our Perf DDRITs will likely catch it. (or, miss it, if STJ happened to have already loaded for other reasons, but we're actively trying to consolidate serialization technologies to reduce wasted assembly loads).
We also have at least one test that uses AppDomains to verify that we don't load unrelated serialization libraries, but IIRC it's off in our PR builds because it's not entirely stable.

StreamJsonRpc has 3 serialization library dependencies (Newtonsoft.Json, MessagePack-CSharp, and STJ). And it may pick up a 4th by the end of the year. It's unfortunate that in .NET we have to choose between having 3 assemblies (StreamJsonRpc, a serialization library, plus a binding library) or taking a hard dependency in StreamJsonRpc to reduce this to 2 assemblies. I know AOT resolves this, for those apps that can do AOT and for libraries that are AOT safe (StreamJsonRpc is not -- yet). So we try to get the best of both worlds by taking the hard dependency and then avoiding references to it outside of code that really requires it.

The NetArchTest.Rules nuget package includes the ability to functionally prove that certain types or namespaces take no dependencies on other areas (e.g. assemblies). I only recently discovered it and it may help us enforce dependency compartmentalizing for StreamJsonRpc via our functional unit tests.

};
}
}
Expand All @@ -45,7 +47,10 @@ public static Func<string, string> Prepend(string prefix)
{
return name => name;
}

return name => prefix + name;
else
{
string localPrefix = prefix;
return name => localPrefix + name;
}
}
}
13 changes: 1 addition & 12 deletions src/StreamJsonRpc/HeaderDelimitedMessageHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ unsafe int WriteHeaderText(string value, Span<byte> memory)
var mediaType = MediaTypeHeaderValue.Parse(contentTypeAsText);
if (mediaType.CharSet is not null)
{
// The common language server protocol accpets 'utf8' as a valid charset due to an early bug. To maintain backwards compatibility, 'utf8' will be
// The common language server protocol accepts 'utf8' as a valid charset due to an early bug. To maintain backwards compatibility, 'utf8' will be
// accepted here so StreamJsonRpc can be used to support remote language servers following common language protocol.
if (mediaType.CharSet.Equals("utf8", StringComparison.OrdinalIgnoreCase))
{
Expand All @@ -338,17 +338,6 @@ unsafe int WriteHeaderText(string value, Span<byte> memory)
}
}

private static bool IsLastFourBytesCrlfCrlf(byte[] buffer, int lastIndex)
{
const byte cr = (byte)'\r';
const byte lf = (byte)'\n';
return lastIndex >= 4
&& buffer[lastIndex - 4] == cr
&& buffer[lastIndex - 3] == lf
&& buffer[lastIndex - 2] == cr
&& buffer[lastIndex - 1] == lf;
}

private static int GetContentLength(ReadOnlySequence<byte> contentLengthValue)
{
// Ensure the length is reasonable so we don't blow the stack if we execute the stackalloc path.
Expand Down
2 changes: 1 addition & 1 deletion src/StreamJsonRpc/Hex.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace StreamJsonRpc;

internal static class Hex
{
private static readonly byte[] HexBytes = new byte[] { (byte)'0', (byte)'1', (byte)'2', (byte)'3', (byte)'4', (byte)'5', (byte)'6', (byte)'7', (byte)'8', (byte)'9', (byte)'a', (byte)'b', (byte)'c', (byte)'d', (byte)'e', (byte)'f' };
private static readonly byte[] HexBytes = "0123456789abcdef"u8.ToArray();
private static readonly byte[] ReverseHexDigits = BuildReverseHexDigits();

internal static void Encode(ReadOnlySpan<byte> src, ref Span<char> dest)
Expand Down
4 changes: 4 additions & 0 deletions src/StreamJsonRpc/JsonRpc.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2601,10 +2601,14 @@ private async Task HandleRpcAsync(JsonRpcMessage rpc)

lock (this.dispatcherMapLock)
{
#if NET
this.resultDispatcherMap.Remove(resultOrError.RequestId, out data);
#else
if (this.resultDispatcherMap.TryGetValue(resultOrError.RequestId, out data))
{
this.resultDispatcherMap.Remove(resultOrError.RequestId);
}
#endif
}

if (this.TraceSource.Switch.ShouldTrace(TraceEventType.Information))
Expand Down
3 changes: 2 additions & 1 deletion src/StreamJsonRpc/LengthHeaderMessageHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Buffers;
using System.Buffers.Binary;
using System.IO.Pipelines;
using Nerdbank.Streams;
using StreamJsonRpc.Protocol;
Expand Down Expand Up @@ -109,7 +110,7 @@ protected override void Write(JsonRpcMessage content, CancellationToken cancella
}

// Now go back and fill in the header with the actual content length.
Utilities.Write(this.Writer.GetSpan(sizeof(int)), checked((int)contentSequence.Length));
BinaryPrimitives.WriteInt32BigEndian(this.Writer.GetSpan(sizeof(int)), checked((int)contentSequence.Length));
Copy link
Member Author

@stephentoub stephentoub Mar 4, 2025

Choose a reason for hiding this comment

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

Is this another one that's going to cause you to load a library you didn't want to?

Hmm, probably not, since you're already using span.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking. Ya, we're using System.Memory heavily throughout the library.

this.Writer.Advance(sizeof(int));
contentSequence.CopyTo(this.Writer);

Expand Down
8 changes: 6 additions & 2 deletions src/StreamJsonRpc/MessagePackFormatter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -402,13 +402,17 @@ private static void WriteProtocolVersionPropertyAndValue(ref MessagePackWriter w
}
}

private static void ReadUnknownProperty(ref MessagePackReader reader, ref Dictionary<string, ReadOnlySequence<byte>>? topLevelProperties, ReadOnlySpan<byte> stringKey)
private static unsafe void ReadUnknownProperty(ref MessagePackReader reader, ref Dictionary<string, ReadOnlySequence<byte>>? topLevelProperties, ReadOnlySpan<byte> stringKey)
{
topLevelProperties ??= new Dictionary<string, ReadOnlySequence<byte>>(StringComparer.Ordinal);
#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER
string name = Encoding.UTF8.GetString(stringKey);
#else
string name = Encoding.UTF8.GetString(stringKey.ToArray());
string name;
fixed (byte* stringKeyPtr = stringKey)
{
name = Encoding.UTF8.GetString(stringKeyPtr, stringKey.Length);
}
#endif
topLevelProperties.Add(name, GetSliceForNextToken(ref reader));
}
Expand Down
5 changes: 1 addition & 4 deletions src/StreamJsonRpc/Protocol/TraceParent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,7 @@ public override string ToString()

Debug.Assert(traceParentRemaining.Length == 0, "Characters were not initialized.");

fixed (char* pValue = traceparent)
{
return new string(pValue, 0, traceparent.Length);
}
return traceparent.ToString();

static void AddHyphen(ref Span<char> value)
{
Expand Down
4 changes: 2 additions & 2 deletions src/StreamJsonRpc/ProxyGeneration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ internal static TypeInfo Get(Type contractInterface, ReadOnlySpan<Type> addition
string rpcMethodName = methodNameMap.GetRpcMethodName(method);
if (rpcInterfaceCode.HasValue)
{
methodName = rpcInterfaceCode + "." + method.Name;
rpcMethodName = rpcInterfaceCode + "." + rpcMethodName;
methodName = $"{rpcInterfaceCode.GetValueOrDefault()}.{method.Name}";
rpcMethodName = $"{rpcInterfaceCode.GetValueOrDefault()}.{rpcMethodName}";
}
Comment on lines 295 to 299

Choose a reason for hiding this comment

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

                    if (rpcInterfaceCode is { } localRpcInterfaceCode)
                    {
                        methodName = $"{localRpcInterfaceCode}.{method.Name}";
                        rpcMethodName = $"{localRpcInterfaceCode}.{rpcMethodName}";
                    }

/cc @stephentoub 😄

Copy link
Member

Choose a reason for hiding this comment

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

Or why not just:

                    if (rpcInterfaceCode.HasValue)
                    {
                        methodName = $"{rpcInterfaceCode}.{method.Name}";
                        rpcMethodName = $"{rpcInterfaceCode}.{rpcMethodName}";
                    }

Copy link
Member Author

Choose a reason for hiding this comment

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

For the latter, just because nullable value types are a bit less efficient in interpolated strings. Probably not a big deal, though.

For the former, seems like a stylistic preference.

Copy link

@paulomorgado paulomorgado Mar 18, 2025

Choose a reason for hiding this comment

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

It's exactly the same code, but more CSharpy. 😄

It just shows you were used to doing the right thing before C# had pattern matching. 😄


ParameterInfo[] methodParameters = method.GetParameters();
Expand Down
4 changes: 4 additions & 0 deletions src/StreamJsonRpc/StandardCancellationStrategy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,14 @@ protected void CancelInboundRequest(RequestId id)
CancellationTokenSource? cts;
lock (this.inboundCancellationSources)
{
#if NET
this.inboundCancellationSources.Remove(id, out cts);
#else
if (this.inboundCancellationSources.TryGetValue(id, out cts))
{
this.inboundCancellationSources.Remove(id);
}
#endif
}

if (cts is object)
Expand Down
92 changes: 2 additions & 90 deletions src/StreamJsonRpc/Utilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Buffers;
using System.Buffers.Binary;
using System.Diagnostics.CodeAnalysis;

namespace StreamJsonRpc;
Expand All @@ -18,39 +19,7 @@ internal static int ReadInt32BE(ReadOnlySequence<byte> sequence)
sequence = sequence.Slice(0, 4);
Span<byte> stackSpan = stackalloc byte[4];
sequence.Slice(0, 4).CopyTo(stackSpan);
return ReadIntBE(stackSpan);
}

/// <summary>
/// Reads an <see cref="int"/> value to a buffer using big endian.
/// </summary>
/// <param name="buffer">The buffer to read from. Must be at most 4 bytes long.</param>
/// <returns>The read value.</returns>
internal static int ReadIntBE(ReadOnlySpan<byte> buffer)
{
Requires.Argument(buffer.Length <= 4, nameof(buffer), "Int32 length exceeded.");

int local = 0;
for (int offset = 0; offset < buffer.Length; offset++)
{
local <<= 8;
local |= buffer[offset];
}

return local;
}

/// <summary>
/// Writes an <see cref="int"/> value to a buffer using big endian.
/// </summary>
/// <param name="buffer">The buffer to write to. Must be at least 4 bytes long.</param>
/// <param name="value">The value to write.</param>
internal static void Write(Span<byte> buffer, int value)
{
buffer[0] = (byte)(value >> 24);
buffer[1] = (byte)(value >> 16);
buffer[2] = (byte)(value >> 8);
buffer[3] = (byte)value;
return BinaryPrimitives.ReadInt32BigEndian(stackSpan);
}

/// <summary>
Expand All @@ -68,61 +37,4 @@ internal static void CopyTo<T>(this in ReadOnlySequence<T> sequence, IBufferWrit
writer.Write(segment.Span);
}
}

/// <summary>
/// Converts a PascalCase identifier to camelCase.
/// </summary>
/// <param name="identifier">The identifier to convert to camcelCase.</param>
/// <returns>The camelCase identifier. Null if <paramref name="identifier"/> is null.</returns>
/// <devremarks>
/// Originally taken from <see href="https://github.com/JamesNK/Newtonsoft.Json/blob/666d9760719e5ec5b2a50046f7dbd6a1267c01c6/Src/Newtonsoft.Json/Utilities/StringUtils.cs#L155-L194">Newtonsoft.Json</see>.
/// </devremarks>
[return: NotNullIfNotNull("identifier")]
internal static string? ToCamelCase(string? identifier)
{
if (identifier is null || identifier.Length == 0 || !char.IsUpper(identifier[0]))
{
return identifier;
}

char[] chars = ArrayPool<char>.Shared.Rent(identifier.Length);
identifier.CopyTo(0, chars, 0, identifier.Length);
try
{
for (int i = 0; i < identifier.Length; i++)
{
if (i == 1 && !char.IsUpper(chars[i]))
{
break;
}

bool hasNext = i + 1 < identifier.Length;
if (i > 0 && hasNext && !char.IsUpper(chars[i + 1]))
{
// if the next character is a space, which is not considered uppercase
// (otherwise we wouldn't be here...)
// we want to ensure that the following:
// 'FOO bar' is rewritten as 'foo bar', and not as 'foO bar'
// The code was written in such a way that the first word in uppercase
// ends when if finds an uppercase letter followed by a lowercase letter.
// now a ' ' (space, (char)32) is considered not upper
// but in that case we still want our current character to become lowercase
if (char.IsSeparator(chars[i + 1]))
{
chars[i] = char.ToLowerInvariant(chars[i]);
}

break;
}

chars[i] = char.ToLowerInvariant(chars[i]);
}

return new string(chars, 0, identifier.Length);
}
finally
{
ArrayPool<char>.Shared.Return(chars);
}
}
}