New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding MsgPack HubProtocol #587

Merged
merged 1 commit into from Jun 28, 2017

Conversation

Projects
None yet
6 participants
@moozzyk
Contributor

moozzyk commented Jun 23, 2017

MsgPackHubProtocol - not plugged in yet.

@@ -16,6 +16,7 @@
<PackageReference Include="Newtonsoft.Json" Version="$(JsonNetVersion)" />
<PackageReference Include="System.Buffers.Primitives" Version="$(CoreFxLabsVersion)" />
<PackageReference Include="System.Binary" Version="$(CoreFxLabsVersion)" />
<PackageReference Include="MsgPack.Cli" Version="0.9.0-beta2" />

This comment has been minimized.

@moozzyk

moozzyk Jun 23, 2017

Contributor

@Eilon - can we get this dependency approved?

This comment has been minimized.

@davidfowl

davidfowl Jun 24, 2017

Member

@moozzyk Can you put this in dependencies.props?

This comment has been minimized.

@moozzyk

moozzyk Jun 24, 2017

Contributor

I will.

msgPackException = e;
}
throw new FormatException($"Reading '{field}' as String failed.", msgPackException);

This comment has been minimized.

@davidfowl

davidfowl Jun 24, 2017

Member

Why not throw directly from the catch?

This comment has been minimized.

@davidfowl

davidfowl Jun 24, 2017

Member

Also, is there a specific exception type?

This comment has been minimized.

@moozzyk

moozzyk Jun 24, 2017

Contributor

ReadString may return false if we are at the end of the stream and there is nothing to read. We should throw in this case because we require the value to be there. ReadString also throws if the string is corrupted or the data to read is not string. We don't want to throw this exception because it is a proprietary exception (MessagePackException). Rather it will be an inner exception.

This comment has been minimized.

@anurse

anurse Jun 28, 2017

Member

I think it'd be clearer to have two throws though. Plus we can have a clearer exception message as to why the read failed.

This comment has been minimized.

@moozzyk

moozzyk Jun 28, 2017

Contributor

The inner exception will tell the details - it is MsgPack specific. If we were at the end of the stream and could not read there won't be any exception. This basically should not happen unless the other side sent a broken message.

I might just add comment in the code.

public bool TryParseMessages(ReadOnlySpan<byte> input, IInvocationBinder binder, out IList<HubMessage> messages)
{
messages = new List<HubMessage>();

This comment has been minimized.

@davidfowl

davidfowl Jun 24, 2017

Member

All it needs is the BinaryMessageParser created here to parse length prefixed messages.

This comment has been minimized.

@moozzyk

moozzyk Jun 24, 2017

Contributor

yes

@davidfowl

This comment has been minimized.

Member

davidfowl commented Jun 24, 2017

Are you going to update the spec?

@moozzyk

This comment has been minimized.

Contributor

moozzyk commented Jun 24, 2017

Yes, I will update the spec separately.

using (var memoryStream = new MemoryStream())
{
_hubProtocol.TryWriteMessage(hubMessage, memoryStream);
memoryStream.Position = 0;

This comment has been minimized.

@davidfowl

davidfowl Jun 24, 2017

Member

This isn't required since you do ToArray()

@neuecc

This comment has been minimized.

neuecc commented Jun 27, 2017

Is the msgpack data is valid?
I think it packed 5 separated message block.

packer.Pack(InvocationMessageType);
packer.PackString(invocationMessage.InvocationId);
packer.Pack(invocationMessage.NonBlocking);
packer.PackString(invocationMessage.Target);
packer.PackObject(invocationMessage.Arguments);

The debugging performance of MessagePack is degraded.

// dump result only returns '1' (InvocationMessageType)

// MsgPack-Cli
Console.WriteLine(MsgPack.Serialization.MessagePackSerializer.UnpackMessagePackObject(stream).ToString());

// MessagePack for C#
Console.WriteLine(MessagePack.MessagePackSerializer.ToJson(stream.ToArray()));

Message should use array header.

packer.PackArrayHeader(5);
packer.Pack(InvocationMessageType);
packer.PackString(invocationMessage.InvocationId);
packer.Pack(invocationMessage.NonBlocking);
packer.PackString(invocationMessage.Target);
packer.PackObject(invocationMessage.Arguments);

It can dump, can read from deserializer and it's just 1 byte.

@neuecc

This comment has been minimized.

neuecc commented Jun 27, 2017

Although it may be unnecessary, this is an example of WriteMessage when described with MessagePack for C#.

MessagePackBinary.WriteArrayHeader(output, 5);
MessagePackBinary.WriteInt32(output, InvocationMessageType);
MessagePackBinary.WriteString(output, invocationMessage.InvocationId);
MessagePackBinary.WriteBoolean(output, invocationMessage.NonBlocking);
MessagePackBinary.WriteString(output, invocationMessage.Target);

MessagePackBinary.WriteArrayHeader(output, invocationMessage.Arguments.Length);
for (int i = 0; i < invocationMessage.Arguments.Length; i++)
{
    if (invocationMessage.Arguments[i] == null)
    {
        MessagePackBinary.WriteNil(output);
    }
    else
    {
        // when WriteMessage can get IInvocationBinder.GetParameters?
        // GetType from argument directly(concrete type) is not good way(lost interface declaration)
        MessagePack.MessagePackSerializer.NonGeneric.Serialize(invocationMessage.Arguments[i].GetType(), output, invocationMessage.Arguments[i]);
    }
}
@moozzyk

This comment has been minimized.

Contributor

moozzyk commented Jun 27, 2017

@neuecc - we prefix the message data with the message length and we write and read/interpret the values so I did not make the message an array. Note that you are not really able to deserialize these messages using high level deserializer since each of these messages may contain objects whose types can only be known after we start reading the invocation id.

I started with MessagePack for C# but was blocked by neuecc/MessagePack-CSharp#63 and eventually went with the MsgPack-Cli. I kept MsgPack usage contained so that it is easy to replace. It should also make it easy to compare the two MessagePack libraries in our scenarios.

{
public class CustomObject //: IEquatable<CustomObject>
{
// Not intended to be a full set of things, just a smattering of sample serializations

This comment has been minimized.

@mikaelm12

mikaelm12 Jun 27, 2017

Contributor

+1 for use of "smattering"

This comment has been minimized.

@moozzyk

moozzyk Jun 27, 2017

Contributor

Credits should go to @anurse - I just moved this code to a separate file

using (var memoryStream = new MemoryStream(input.ToArray()))
{
messages.Add(ParseMessage(memoryStream, binder));

This comment has been minimized.

@mikaelm12

mikaelm12 Jun 27, 2017

Contributor

What happens if this Parse fails?

This comment has been minimized.

@mikaelm12

mikaelm12 Jun 27, 2017

Contributor

Oh, parse message just throws

}
// TODO: when to return false?
public bool TryWriteMessage(HubMessage message, Stream output)

This comment has been minimized.

@mikaelm12

mikaelm12 Jun 27, 2017

Contributor

Do we need TryWriteMessage method wrapping WriteMessage? You throw in the failure case

This comment has been minimized.

@moozzyk

moozzyk Jun 27, 2017

Contributor

They could be merged indeed. I am planning to review the Try* methods we have since they oftentimes never return false (anymore).

This comment has been minimized.

@anurse

anurse Jun 28, 2017

Member

The main reason for them is around transitioning away from Stream to Span/IOutput based APIs. Until we have a clear picture for how those will work, we could just replace them with non-Try versions. The only time these Try APIs are supposed to return false is when there isn't enough space to write in the buffer allocated to the method, or there isn't enough data to read in the buffer provided.

namespace Microsoft.AspNetCore.SignalR.Common.Tests.Internal.Protocol
{
public class CustomObject //: IEquatable<CustomObject>

This comment has been minimized.

@mikaelm12

mikaelm12 Jun 27, 2017

Contributor

Remove comment?

This comment has been minimized.

@moozzyk

moozzyk Jun 27, 2017

Contributor

Need to uncomment actually.

}
}

This comment has been minimized.

@mikaelm12

mikaelm12 Jun 27, 2017

Contributor

nit: Remove extra newline

var rightEnumerator = rightEnumerable.GetEnumerator();
var leftMoved = leftEnumerator.MoveNext();
var rightMoved = rightEnumerator.MoveNext();
for (; leftMoved && rightMoved; leftMoved = leftEnumerator.MoveNext(), rightMoved = rightEnumerator.MoveNext())

This comment has been minimized.

@mikaelm12

This comment has been minimized.

@moozzyk

moozzyk Jun 27, 2017

Contributor

move iterators in tandem

}
}
return !leftMoved && !rightMoved;

This comment has been minimized.

@mikaelm12

mikaelm12 Jun 27, 2017

Contributor

nit: You can simplify this from !A && !B to !(A && B)

This comment has been minimized.

@moozzyk

moozzyk Jun 27, 2017

Contributor

no, as per De Morgan law it would be !(A || B). I had it but I reverted since I find it cleaner.

This comment has been minimized.

@mikaelm12

mikaelm12 Jun 27, 2017

Contributor

😅

This comment has been minimized.

@anurse

@moozzyk moozzyk merged commit 3504337 into dev Jun 28, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@moozzyk moozzyk deleted the pawelka/msgpack branch Jun 28, 2017

@@ -17,7 +17,7 @@ public static byte[] WriteToArray(this IHubProtocol protocol, HubMessage message
{
throw new InvalidOperationException("Failed to write message to the output stream");
}

This comment has been minimized.

@anurse

anurse Jun 28, 2017

Member

Trailing Space Flagger strikes again! :)

}
// TODO: when to return false?
public bool TryWriteMessage(HubMessage message, Stream output)

This comment has been minimized.

@anurse

anurse Jun 28, 2017

Member

The main reason for them is around transitioning away from Stream to Span/IOutput based APIs. Until we have a clear picture for how those will work, we could just replace them with non-Try versions. The only time these Try APIs are supposed to return false is when there isn't enough space to write in the buffer allocated to the method, or there isn't enough data to read in the buffer provided.

msgPackException = e;
}
throw new FormatException($"Reading '{field}' as String failed.", msgPackException);

This comment has been minimized.

@anurse

anurse Jun 28, 2017

Member

I think it'd be clearer to have two throws though. Plus we can have a clearer exception message as to why the read failed.

namespace Microsoft.AspNetCore.SignalR.Common.Tests.Internal.Protocol
{
public class CustomObject : IEquatable<CustomObject>

This comment has been minimized.

@anurse

anurse Jun 28, 2017

Member

Now that this is separate from the test file, can we call it TestCustomObject or something?

This comment has been minimized.

@moozzyk

moozzyk Jun 28, 2017

Contributor

Sure.

}
}
return !leftMoved && !rightMoved;

This comment has been minimized.

@anurse
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment