Skip to content
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

F# API - problem with discriminated union serialization #999

Closed
Horusiath opened this issue May 21, 2015 · 30 comments · Fixed by #1203
Closed

F# API - problem with discriminated union serialization #999

Horusiath opened this issue May 21, 2015 · 30 comments · Fixed by #1203

Comments

@Horusiath
Copy link
Contributor

@neoeinstein found a problem with DU serialization/deserialization:

I tried passing a TraceableMessage, which failed complaining about receiving the wrong token in the JSON. Then I tried unwrapping things and just sending over the ItemAvailabilityMessage, but that gave be the NullReferenceException on remote deserialize.

Gist with message types: https://gist.github.com/neoeinstein/30a38229c368313b721d

@erdeszt
Copy link

erdeszt commented May 27, 2015

Having the same issue. Sending discriminated unions to remotes fail with NullReferenceException (even with a simple Option type). Works fine with local actors.
I'm running mono 4.1 and f# 3.0 on linux mint 17(x64 the build target is x86).
Example code: https://gist.github.com/erdeszt/aece58b48bc4ebfe7090
Edit: Updated the gist to show that this is not a json.net issue.

@rogeralsing
Copy link
Contributor

I suspect that the actual serialization works, but that when we put that into the remoting envelopes we might lack the type information to actually deserialize it on the other end.

I will look into it

@rogeralsing rogeralsing self-assigned this May 29, 2015
@Horusiath
Copy link
Contributor Author

@erdeszt This is JSON.NET issue (to be exact it's < 7.0 version issue) - your example doesn't apply to Akka.NET, since it uses default settings (akka serializer uses custom configuration, which provokes the problem). To fix this you may upgrade Newtonsoft.Json to version >= 7 .

Also I've created a custom fork of Akka.FSharp called Akkling, which solves that issue by upgrading to the newest JSON.NET by default.

@erdeszt
Copy link

erdeszt commented Jun 18, 2015

@Horusiath thanks for the info and sorry for my confusion, I didn't know about the custom configuration of Json.Net.
Is there a chance for a patch release with the upgraded Json.Net lib? I'm not that keen on using my own fork or experimental libs.
Not that I don't like your approach I was also playing around with the F# api to make it more typesafe, so I'd be happy to see your changes in the official Akka.FSharp repo. Any plans on merging it when you have a stable version?

@Horusiath
Copy link
Contributor Author

I don't suspect to merge it into Akka.FSharp any time soon, since it contains some breaking changes to the API, and we can't afford to make them since library is already officially 1.0. I've decided to make independent lib, which is not bound by core Akka tooling (we can use F# specific tools like i.e. Paket or Project Scaffold this way) and release cycle.

@Aaronontheweb
Copy link
Member

Is there a chance for a patch release with the upgraded Json.Net lib? I'm not that keen on using my own fork or experimental libs.

Yes, if JSON.NET 7.0 is stable then we will upgrade Akka.NET to use it.

@erdeszt
Copy link

erdeszt commented Jun 18, 2015

@Horusiath @Aaronontheweb thanks for the answers!

@lepinay
Copy link

lepinay commented Jun 30, 2015

Is there a workaround for this ? I've tried a binding redirect to JSON.NET 7.0 but still got the issue.
(null reference @ Akka.Serialization.NewtonSoftJsonSerializer.FromBinary)

@Horusiath
Copy link
Contributor Author

@lepinay could you paste a stack trace?

@lepinay
Copy link

lepinay commented Jun 30, 2015

[ERROR][30/06/2015 20:37:09][Thread 0018][[akka://MyTarget/system/endpointManager/reliableEndpointWriter-akka.tcp%3a%2f%2fMySystem%40localhost%3a8091-1/endpointWriter]] AssociationError [akka.tcp://MyTarget@localhost:8090] <- akka.tcp://MySystem@localhost:8091: Error [La référence d'objet n'est pas définie à une instance d'un objet.] [ à Akka.Serialization.NewtonSoftJsonSerializer.TranslateSurrogate(Object deserializedValue, ActorSystem system, Type type)
à Akka.Serialization.NewtonSoftJsonSerializer.FromBinary(Byte[] bytes, Type type)
à Akka.Remote.MessageSerializer.Deserialize(ActorSystem system, SerializedMessage messageProtocol)
à Akka.Remote.DefaultMessageDispatcher.Dispatch(IInternalActorRef recipient, Address recipientAddress, SerializedMessage message, IActorRef senderOption)
à Akka.Remote.EndpointReader.OnReceive(Object message)
à Akka.Actor.UntypedActor.Receive(Object message)
à Akka.Actor.ActorBase.AroundReceive(Receive receive, Object message)
à Akka.Actor.ActorCell.ReceiveMessage(Object message)
à Akka.Actor.ActorCell.Invoke(Envelope envelope)]

@Horusiath
Copy link
Contributor Author

Could you paste also a type, you're trying to serialize? ;)

@lepinay
Copy link

lepinay commented Jun 30, 2015

Yes sure :)

type ResultCategory = 
    | Men
    | Women

type LinkGeneratorMessage =
    | StartBatch of int*int*string*ResultCategory // This is the message I send
    | Result of string list*ResultCategory
    | LinkParsed of string*Result
    | ParseError of string*exn
    | Error

Also, just to be complete, I'm testing this inside fsharp interactive, I've patched up fsi.exe.config with

      <dependentAssembly>
        <assemblyIdentity name="Newtonsoft.Json" publicKeyToken="30ad4fe6b2a6aeed" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-7.0.0.0" newVersion="7.0.0.0" />
      </dependentAssembly>

and actors running in the local system (akka.tcp://MySystem@localhost:8091) can process the message without any issues

@Horusiath
Copy link
Contributor Author

Ok, the reason is here. F# can compile discriminated union either to form of class hierarchy or enum, but our hack around JSON.NET doesn't check the second case. I don't know if we solve it as a hotfix, probably it would be better to upgrade JSON.NET version dependency in next release and get rid of this code.

cc @akkadotnet/developers

@rogeralsing
Copy link
Contributor

This test should verify if things work correctly.
It wraps the DU in an object array.

In the current impl, we cannot see what type the DU is when it is stored in an object or object array as it doesnt carry any type information by itself.
So we resolve that from the containing property. but if the container property is object or object array, we have no information to use.

[<Fact>]
let ``can serialize discriminated unions in object container`` () =
    let x = B (23,"hello")
    let obj = [| x :> System.Object  |]
    use sys = System.create "system" (Configuration.defaultConfig())
    let serializer = sys.Serialization.FindSerializerFor obj
    let bytes = serializer.ToBinary obj
    let des = serializer.FromBinary (bytes, typeof<obj[]>) :?> obj[]
    let desx = des.[0] :?> TestUnion
    desx
    |> equals x

I'm trying this with Json.NET 7 now and this is the resulting json:

{"$type":"System.Object[], mscorlib","$values":[{"Case":"B","Fields":[{"$":"I23"},"hello"]}]}

As you can see, there is no type information inside the $values property either.
So we are still not able to handle this.

Even if we add the new DiscriminatedUnionConverter to our Json.NET serializer settings

And incase anyone wonders why we want to wrap in object array, we use object arrays for argument passing when remote deploying actors for example. ctor arguments.

@forki
Copy link
Contributor

forki commented Jul 1, 2015

/cc @eiriktsarpalis any ideas?

@lepinay
Copy link

lepinay commented Jul 5, 2015

Now I'm torn between converting all my fsharp code to the csharp API or waiting for a fix (I wish I could help but I'm have no clue on how to fix this)...is there any kind of work around even a not efficient one ? Would .NET default binary or xml serializer work ?

@Horusiath
Copy link
Contributor Author

@lepinay One of the workarounds is to create a marker interface, attach it to those F# types, that have to be serialized - only top message types. Then create a custom message serializer and associate it with this marker interface.

Here is working example using FsPickler:

// marker interface
type ISerialMessage = interface end

let internal serializeToBinary (fsp:BinarySerializer) o = 
        use stream = new System.IO.MemoryStream()
        fsp.Serialize(o.GetType(), stream, o)
        stream.ToArray()

let internal deserializeFromBinary (fsp:BinarySerializer) (bytes: byte array) (t: Type) =
        use stream = new System.IO.MemoryStream(bytes)
        fsp.Deserialize(t, stream)

// used for top level serialization
type FsSerializer(system) = 
    inherit Serializer(system)
    let fsp = FsPickler.CreateBinary()
    override __.Identifier = 1001 // this value must be unique (over 100)
    override __.IncludeManifest = true        
    override __.ToBinary(o) = serializeToBinary fsp o        
    override __.FromBinary(bytes, t) = deserializeFromBinary fsp bytes t :> obj

// EXAMPLE
type ResultCategory = 
    | Men
    | Women

type LinkGeneratorMessage =
    | StartBatch of int*int*string*ResultCategory // this was problematic type
    interface ISerialMessage // only top level messages have to be marked

let system = Configuration.defaultConfig() |> System.create "SerializableSystem"
// create serializer and associate it with ISerialMessage
let fsharpSerializer = FsSerializer(system :?> Akka.Actor.ExtendedActorSystem)
system.Serialization.AddSerializer(fsharpSerializer)
system.Serialization.AddSerializationMap(typeof<ISerialMessage>, fsharpSerializer)

// proof
let x = StartBatch(1, 2, "str", Men)
printfn "Before: %A" x
let serializer = system.Serialization.FindSerializerFor x
let binary = serializer.ToBinary x
let y = serializer.FromBinary(binary, x.GetType())
printfn "After: %A" y

@lepinay
Copy link

lepinay commented Jul 6, 2015

@Horusiath Thanks for showing the way ;)

@lepinay
Copy link

lepinay commented Jul 9, 2015

Work around works, except for part of my system where I use routers, messages are not delivered to routees (dead letters).

Will #1128 fix this or is it a separeate issue with the fsharp api and remote routers deployment ?

let router =  spawnOpt mailbox "router" emptyActor [SpawnOption.Router(Routing.RoundRobinGroup([parser "1";parser "2"]))]

If I get rid of router and talk to parser "1" directly, then things work normally.

@Horusiath
Copy link
Contributor Author

@lepinay Roger's update should fix this one.

@rogeralsing
Copy link
Contributor

As this has been merged, I'm closing this

@Horusiath Horusiath reopened this Aug 3, 2015
@rogeralsing
Copy link
Contributor

Is this still an issue? @Horusiath

@Horusiath
Copy link
Contributor Author

After discussion with @lepinay over the gitter, it still seems to be an issue. However needs confirmation.

@Aaronontheweb
Copy link
Member

@Horusiath even with JSON.NET 7? Latest off of dev?

@CumpsD
Copy link
Member

CumpsD commented Dec 27, 2015

Should this work?

I am getting this:
[WARNING][27/12/2015 12:22:21][Thread 0011][Akka.Routing.ConsistentHashingRoutingLogic] Message [JObject] must be handled by hashMapping, or implement [IConsistentHashable] or be wrapped in [ConsistentHashableEnvelope]

Sending in something like:

let loginCommand = Login { LoginCommand.Email = Email.create "test@test.be" |> Option.get; Password = Password.create "test123456789" |> Option.get }

Simplified DU:

    type Command =
    | Login of LoginCommand

    and LoginCommand = {
        Email: Email.Email
        Password: Password.Password
    }

@CumpsD
Copy link
Member

CumpsD commented Dec 27, 2015

PS: I am using Newtonsoft.Json (7.0.1)

@Horusiath
Copy link
Contributor Author

@CumpsD Error suggests that you're using router with consistent hashing. In order to send messages to it, they need to implement IConsistentHashable interface - just like in error, you've received.

@CumpsD
Copy link
Member

CumpsD commented Dec 27, 2015

The error actually said must be handled by hashMapping or

Which I did:

        let userHash = ConsistentHashingPool 5
        let userHash = userHash.WithHashMapping(fun msg ->
            match msg with
            | :? Commands.Command as cmd ->
                match cmd with
                | Login userCommand -> userCommand.Email |> Email.value :> obj
                | _ -> null
            | _ -> null
        )
        let userActor = spawnOpt system "user" (actorOf userActor) [SpawnOption.Router(userHash)]

However, the incoming msg gets turned into a JObject, which apparently was the issue of this GH issue.

@rogeralsing
Copy link
Contributor

@CumpsD you are using Akka.Remote or Akka.Cluster?
Consistent Hash Routing does use the serializer even in non remote scenarios, so I just want to make sure I get the full context here.

@CumpsD
Copy link
Member

CumpsD commented Dec 27, 2015

I am using Akka.Remote

On Dec 27, 2015 8:39 PM, "Roger Johansson" notifications@github.com wrote:

@CumpsD you are using Akka.Remote or Akka.Cluster?
Consistent Hash Routing does use the serializer even in non remote
scenarios, so I just want to make sure I get the full context here.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants