Changing msgpack options

[tebeco]

Summary of the changes :

• In MessagePack-CSharp v2 there is a MessagePackSerializerOptions class passed to the serializer
• This PR try to see if it's possible to let the consumer seed an instance of MessagePackSerializerOptions rather than an IList<IFormatterResolvers>

Addresses #18569

First Approach

Use an IOptions<MessagePackSerializerOptions>,
an attempt was done here https://github.com/tebeco/AspNetCore/commit/56c457739c54677626fad11f2821f3c101d8e04c
BUT not possible since this type has no public parameter less ctor AND it's API if a Fluent/Immutable one making it impossible to do this :

builder
    .Services.AddOptions<MessagePackSerializerOptions>()                                       // <=== will crash on first call (no ctor)
    .Configure(options => XXX = options.WithResolver(SignalRResolver.Instance))                // Will have 0 impact since it creates a new instance that wont be tracked by the DI
    .PostConfigure(options => XXX = options.WithSecurity(MessagePackSecurity.UntrustedData));  // same here

Other Approaches

1. Keep the current IOptions<MessagePackHubProtocolOptions> and expose directly a property of type MessagePackSerializerOptions
2. Remove the IOptions<> and create an overload accepting an instance of MessagePackSerializerOptions
   • Not DI friendly
   • then i stumbled on lots of issue, the WIP is here
   • the 1 test KO is the CustomFormatter one, and clearly show DI issue + the use of TryAddEnumerable + captured Closure
   • if we need something in the DI to avoid that closure issue ... then why not an IOptions<> ... oO hello proposal 1

Currently looking at Approach 1

[tebeco]

I can't make pass all the RedisEndToEnd tests from both build.cmd -test and VS2019 16.6 Preview1 :(

I even updates locally ServiceStack.Redis to 2.1.0 that was release yesterday as it contains lots of error message enhancement/fix, still the same result.
Can someone reproduce or make them pass ?

Here is what i see :
I had to put a break point on the failling line : https://github.com/dotnet/aspnetcore/blob/master/src/SignalR/server/StackExchangeRedis/test/RedisEndToEnd.cs#L111

(Even using docker logs --follow <name> does no see anymore activity, until the container is destroy because it's starterd with --rm)

One example of error :

 Message: 
    Microsoft.AspNetCore.SignalR.HubException : An unexpected error occurred invoking 'EchoUser' on the server. RedisConnectionException: No connection is available to service this operation: PUBLISH Microsoft.AspNetCore.SignalR.StackExchangeRedis.Tests.EchoHub:user:userA; UnableToConnect on 172.17.0.2:6379/Interactive, Initializing/NotStarted, last: NONE, origin: BeginConnectAsync, outstanding: 0, last-read: 37s ago, last-write: 37s ago, keep-alive: 60s, state: Connecting, mgr: 10 of 10 available, last-heartbeat: never, global: 0s ago, v: 2.0.593.37019; IOCP: (Busy=0,Free=1000,Min=16,Max=1000), WORKER: (Busy=5,Free=32762,Min=16,Max=32767), Local-CPU: n/a
  Stack Trace: 
    HubConnection.InvokeCoreAsyncCore(String methodName, Type returnType, Object[] args, CancellationToken cancellationToken) line 776
    ForceAsyncAwaiter`1.GetResult() line 64
    HubConnection.InvokeCoreAsync(String methodName, Type returnType, Object[] args, CancellationToken cancellationToken) line 390
    TaskExtensions.TimeoutAfter(Task task, TimeSpan timeout, String filePath, Int32 lineNumber) line 45
    RedisEndToEndTests.CanSendAndReceiveUserMessagesFromMultipleConnectionsWithSameUser(HttpTransportType transportType, String protocolName) line 111
    --- End of stack trace from previous location ---

[halter73]

Why is there no ref/ folder for this project? Is it due to the recent upgrade to MessagePack-CSharp v2?

[analogrelay]

I don't think we build reference assemblies for it since it's not in the shared framework.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[BrennanConroy]

public class MessagePackHubProtocolOptions
{
-   public IList<IFormatterResolver> FormatterResolvers { get; set; }
+   public MessagePackSerializerOptions SerializerOptions { get; set; }
}

We've already made a breaking change this release by upgrading to MessagePack 2.X which changes the API of custom formatters that users can register with the old options.
There is also now a first party class for options for the MessagePack library introduced in 2.X that we would like to use which provides better usage for our customers.

[tebeco]

can someone tag or associate the PR #18133 for upgrading msgpack to v2 in the preview3 release with this one ?

as that PR did not seems to have a proper issue at that time or something else it did not showed up in preview1 release or links containing filters
the associated one seems to be #18074 which might not be the rightful one
i might have forgot to create a dedicated one for that

it might comes as a "surprise" for consumers already using msgpack as it was not part of the release note

[BrennanConroy]

https://docs.microsoft.com/en-us/dotnet/core/compatibility/3.1-5.0#signalr-messagepack-hub-protocol-moved-to-messagepack-2x-package ?

[tebeco]

hummm i did not checked this link, you are absolutely right on this one, i followed the announcement and blog post for preview 1
https://devblogs.microsoft.com/aspnet/asp-net-core-updates-in-net-5-preview-1/

there was a list for breaking change pointing here :
https://github.com/aspnet/announcements/issues?q=is%3Aopen+is%3Aissue+milestone%3A5.0+label%3A%22Breaking+change%22

It seems to be only filtering issue with Breaking change tag which is probably the issue (no issue associated)

my bad

[BrennanConroy]

Well it's new, I think those were added yesterday 😆

[BrennanConroy]

Lets add xml comments.

• Recommend setting .WithSecurity(MessagePackSecurity.UntrustedData) if they override the options
• If you want to modify to the default options you need to assign the options back to the SerializerOptions after modifications:

SerializerOptions = SerializerOptions.WithResolver(new CustomResolver());

[tebeco]

quick question, as the API has been approuved, and thinking about the summary, i have a terrible idea....
This would make another PR i guess since this API Changed has been approuved already ?

(i have no idea if it's possible) but does adding this makes sense:
(pseudo code, i have no idea if it's something good or bad since it's intrusive and also how properly speak english and being both nice and clear to the customer)

if(env.IsDevelopment() && options.serializerOptions.MessagePackSecurity != MessagePackSecurity.UntrustedData)
{
  if(options.EnableWarningForUntrustedData)
  {
    logger.LogWarning("MessagePackSerializerOptions have been overriden with a ... security settings ...blablabla if intended, you can turn this warning off by settings EnableWarningForUntrustedData to false")

   could even points to a github issue explaining that API

  }
}

[AArnott]

The options.serializerOptions.MessagePackSecurity != MessagePackSecurity.UntrustedData would need some massaging because UntrustedData is a copy-on-write object so if they've 'enhanced' it at all, you wouldn't want that to reactivate the failure. You'd want to look into the object to see what settings it has to determine whether it's a secure instance. But let's first wait to see what the ASP.NET Core folks think of your overall idea.

[tebeco]

as said it just something that crossed my mind while trying to find an idea for the summary (TBH I'm pretty bad at it)
Also i have no idea if it's even possible

[tebeco]

(previous rendering deleted)

[tebeco]

[tebeco]

welllll
I should remove the . since the <code> seems to create a paragraph right
@anurse ?

[tebeco]

(pushed a revert commit to this) :

[analogrelay]

Good point. Yep, drop the . after the <code>.

[tebeco]

(done, do i mark this thread as resolved if summary if ok for you ?)

[tebeco]

Do i rebase the branch ?
IIRC since you'll squash it all it does not matter as long as there's no conflict ?

[analogrelay]

Words smithed

[tebeco]

(see #20031 (comment))

[tebeco]

As it look like we reach the end of the PR

my gif contribution of the day ...

[BrennanConroy]

Thanks again @tebeco !

[tebeco]

thx for your patience here ^^

[halter73]

Thanks for your patience. That was a lot of feedback to deal with. 🎉