Expose private members in MsgPack Reader

[Shmew]

I'm trying to implement @kerams's new addition of a MsgPack (de)serializer for Fable.SignalR. I'm mostly bumbling around (as I have no idea what I'm doing with this stuff) trying to interface IHubProtocol which is pretty hard to understand.

I (most likely don't) maybe have something wired up to some extent, but due to how the message parsing is handled I can't just parse the data as a given type.

So what I have so far:

open Microsoft.AspNetCore.SignalR
open Microsoft.AspNetCore.SignalR.Protocol
open Microsoft.AspNetCore.Connections
open System
open System.IO
open System.Buffers

[<RequireQualifiedAccess>]
module MsgPack =
    module BinaryMessageParser =
        let [<Literal>] MaxLengthPrefixSize = 5

        let tryParse (buffer: byref<ReadOnlySequence<byte>>) (payload: byref<ReadOnlySequence<byte>>) =
            if buffer.IsEmpty then
                payload <- Unchecked.defaultof<ReadOnlySequence<byte>>
                false
            else
                let lengthPrefixBuffer : ReadOnlySequence<byte> = buffer.Slice(0L, Math.Min(int64 MaxLengthPrefixSize, buffer.Length))

                let span =
                    if lengthPrefixBuffer.IsSingleSegment then
                        lengthPrefixBuffer.First.Span
                    else ReadOnlySpan<byte>(lengthPrefixBuffer.ToArray())

                let mutable length = 0uy
                let mutable numBytes = 0

                let mutable byteRead = 0uy

                while numBytes < (int lengthPrefixBuffer.Length) && (byteRead &&& 0x80uy) <> 0uy do
                    byteRead <- span.[numBytes]
                    length <- length ||| ((byteRead &&& 0x7fuy) <<< numBytes * 7)
                    numBytes <- numBytes + 1

                if ((byteRead &&& 0x80uy) <> 0uy) && (numBytes < MaxLengthPrefixSize) then
                    payload <- Unchecked.defaultof<ReadOnlySequence<byte>>
                    false
                elif (byteRead &&& 0x80uy) <> 0uy || (numBytes = MaxLengthPrefixSize && byteRead > 7uy) then
                    failwith "Messages over 2GB in size are not supported"
                elif buffer.Length < (int64 length) + int64 numBytes then
                    payload <- Unchecked.defaultof<ReadOnlySequence<byte>>
                    false
                else
                    payload <- buffer.Slice(numBytes, int length)
                    buffer <- buffer.Slice(int64 (numBytes + int length))
                    true

    type FableHubProtocol () =
        let [<Literal>] ProtocolName = "messagepack"
        let [<Literal>] ProtocolVersion = 1

        interface IHubProtocol with
            member _.Name = ProtocolName

            member _.Version = ProtocolVersion

            member _.TransferFormat = TransferFormat.Binary

            member _.IsVersionSupported version = version = ProtocolVersion

            member _.WriteMessage (message: HubMessage, output: IBufferWriter<byte>) = 
                use ms = new MemoryStream()
                let readSpan = new ReadOnlySpan<byte>()

                Fable.Remoting.MsgPack.Write.object message ms
                ms.Write(readSpan)
                readSpan.CopyTo(output.GetSpan())

            member _.TryParseMessage (input: byref<ReadOnlySequence<byte>>, binder: IInvocationBinder, message: byref<HubMessage>) = 
                let mutable payload = ReadOnlySequence<byte>()
                if not (BinaryMessageParser.tryParse &input &payload) then
                    message <- Unchecked.defaultof<HubMessage>
                    false
                else

                    Fable.Remoting.MsgPack.Reader(input.ToArray()). // No way to parse in the way they are forcing me to
                    true

            member _.GetMessageBytes (message: HubMessage) =
                use ms = new MemoryStream()

                Fable.Remoting.MsgPack.Write.object message ms

                ReadOnlyMemory<byte>(ms.GetBuffer())

Essentially I need to create a class that resembles this.

I'm also not sure if I'm even doing the stuff with the memory streams correctly, but I'm just at the stage of trying things to fit the interface and then see what actually happens.

I should add I'm not even sure if the private members will give me what I really need here, I'm pretty clueless (if you can't tell) about this subject.

[kerams]

Ah, so every message is wrapped in some SignalR envelope? That's a bummer. Yeah, I think you'll need to handcraft (de)serialize functions for those classes using the private members you speak of since what I wrote only works with records. Once you have that, you should be able to use Reader.Read on the actual message content dispatched by the client.

[Shmew]

I'm able to at least complete a handshake now, but I'm not sure how to proceed here. I made a branch available if you're willing to take a look. I think something is wrong with how I'm crafting the more complex types as the client complains about an incomplete message. I'm just trying with the signalr client parser, since we can't write (on the client) with your library at the moment.

[kerams]

Hey, @Zaid-Ajaj, on a somewhat related note, @Shmew suggested that the Message Pack implementation would get treeshaken by Webpack if not used. Can you somehow verify it? It would allow us to implement serialization on the Fable side too (for SignalR but also paving the way to having binary serialized API requests) with no concern for bundle size.

[Shmew]

At the very least you can make everything inline and erase the modules. Fable does not compile inline functions unless used.

[Zaid-Ajaj]

suggested that the Message Pack implementation would get treeshaken by Webpack if not used.

I believe the implementation is used, except that it is not executed which is a subtle difference but is very important: the logic that makes the decision on whether or not the binary protocol should be used, is executed in runtime so I am guessing that this information is not statically known for the Fable in order for it to erase the implementation.

Anyways, I shouldn't guess these things so I will try to verify and see whether this is the case :smile:

[Shmew]

Three questions:

• How big is the compiled file for the msgpack support? - Is this actually an issue?
• What about making them open a namespace to opt-in so Fable does know for sure if it's possible to use or not?
• Should we just inline all the calls? Then the compiled file is virtually empty unless called.

[Zaid-Ajaj]

How big is the compiled file for the msgpack support? - Is this actually an issue?

The implementation looks minimal so I don't think it matters "that much" of course we can always squeeze out more bundle size optimizations. The first one of which would be removing Fable.Parsimmon as a dependency from Fable.SimpleJson and we automatically get a nice bundle size decrease (didn't have time to get into that yet)

However, if we get to the original issue, is there something we can do from the pov of the MsgPack library? (To be honest, I have no idea either how to correctly interface the IHubProtocol stuff myself 😞)

[kerams]

If I implement Fable serialization, then the members may supposedly stay private (unless you still want them public). I won't get around to doing that before Monday though.

[Shmew]

yeah if we have fable serialization then it will all just work™ after I do some minor changes to my repo

[Shmew]

I tried another go at using msg pack with SignalR, but ran into some issues.

It looks like complex unions fail to serialize correctly for example if you add this DU to the Fable.Remoting.MsgPack.Tests/Types.fs file:

[<RequireQualifiedAccess>]
type Msg =
    | Invocation of headers:Map<string,string> option * invocationId:string option * target:string * args:int [] * streamIds:string [] option
    | StreamItem of headers:Map<string,string> option * invocationId:string option * item:int option
    | Completion of headers:Map<string,string> option * invocationId:string * error:string option * result:int option
    | StreamInvocation of headers:Map<string,string> option * invocationId:string * target:string * args:int [] * streamIds:string [] option
    | CancelInvocation of headers:Map<string,string> option * invocationId:string option
    | Ping
    | Close of error:string option * allowReconnect:bool option

This assertion will fail:

test "big DU" {
    Msg.Ping |> serializeDeserializeCompare // Pass
    Msg.Invocation(None, Some "testing", "test", [||], None) |> serializeDeserializeCompare // Pass
    Msg.Close(Some "test", None) |> serializeDeserializeCompare // Fail
}

Also, the way Write.fs is currently setup means it's unusable from Fable as the binary was not compiled with the FABLE_COMPILER directive so it gives compiler errors and doesn't populate intellisense. In addition, looking at the code for Fable.Remoting.Client, as far as I can tell it's not actually serializing into message pack currently.

As a side note, this API is super easy to use!

[kerams]

You can use it from Fable like this https://github.com/Zaid-Ajaj/Fable.Remoting/blob/8eaef0716f3ab0220835f146abe0b9ed22c6695f/ClientV2Tests/src/App.fs#L1321-L1332

It's far from ideal. but because of having to work around poorer support of generics and inlining in Fable, the API in the Write module is different between Fable and .NET. I'm not sure there's a solution to this other than splitting the implementation into 2 separate packages.

In addition, looking at the code for Fable.Remoting.Client, as far as I can tell it's not actually serializing into message pack currently.

~What do you mean?~ Oh yeah, remoting requests are still JSON only. I had a brief look at implementing binary, but it seemed too complicated given the questionable benefits. I'd say requests from run-of-the-mill web applications are always tiny and simple, so there's little point in going away from JSON.

I'll take a look at the failing case.

[kerams]

Very bizarre. if you rename error:string option in Completion to blah:string option, the test passes. This could be a TypeShape bug.

[Shmew]

Very bizarre. if you rename error:string option in Completion to blah:string option, the test passes. This could be a TypeShape bug.

I'm very impressed you found that to be the cause so quickly haha.

the API in the Write module is different between Fable and .NET.

Yeah I was aware of that after digging around, the issue is writeObject doesn't exist to libraries not directly referencing the project (like those in this repo).

Currently I just copy and pasted the Fable only part of the Write module, but it's not ideal.

[Shmew]

So I converted the type parameters that were obj to generic types that I pass through during the setup of the service, and removed all the annotations, but I still get Cannot serialize Object.

The weird thing is that it fails on the simplest case of them: Ping, although that is one of the first messages.

[kerams]

If you're doing Write.serializeObj Msg.Ping ms, you need to use a type annotation, otherwise all 3 generic type parameters are going to be inferred as obj, and it's impossible to create a serializer for Msg<obj, obj, obj>.

https://github.com/Zaid-Ajaj/Fable.Remoting/blob/8eaef0716f3ab0220835f146abe0b9ed22c6695f/Fable.Remoting.MsgPack.Tests/FableConverterTests.fs#L59-L61

Alternatively, you can create a serializer yourself and cache it.

let serializer = Write.makeSerializer<'ClientStreamApi, 'ServerApi, 'ServerStreamApi> ()
// ...
serializer.Invoke (Ping, ms)

[Shmew]

It was a lot harder than I expected this all to be, but I've got it using message pack on both the client and server! Thanks a ton for all your help and efforts @kerams!

[kerams]

Don't mention it.

[kerams]

I think the simplest solution to the differing API in the Write module is conditionally compiling bodies, not entire functions. Something like this:

let writeObject (o, typ, buffer) =
    #if !FABLE_COMPILER
    raise (NotSupportedException "This function is meant to be used in Fable, please use serializeObj or makeSerializer.")
    #else
    // the actual implementation
    #endif

let serializeObj (o, ms) =
    #if FABLE_COMPILER
    raise (NotSupportedException "This function is meant to be used in .NET, please use writeObj.")
    #else
    // the actual implementation
    #endif

It might also be worth unifying the 2 functions, but they can't be overloads because they're not methods at the moment.

What do you think @Zaid-Ajaj, @Shmew?

[Shmew]

Doing it in the function body would mean I don't need to copy and paste the write module so LGTM.

It might be worth it to add it into an additional module with required access so they need to explicitly namespace into Fable to use it.

That would mean if they have Fable.Remoting.MsgPack open they'd need to do Fable.Write.writeObject. Then there's little chance someone stumbles into it when writing actual server code.

[Shmew]

BTW message pack support is now live in Fable.SignalR 0.8! I'd love it if you could review and see if there's something blatantly wrong that could affect performance if you are ever feeling bored.