philippseith / signalr

SignalR server and client in go
MIT License
140 stars 40 forks source link

Cannot create pull stream / missing method #65

Closed andig closed 3 years ago

andig commented 3 years ago

I'm trying to rebuild this example using the go client: https://developer.easee.cloud/page/signalr-code-examples

I'm assuming (I'm new to to SignalR) that on is synonym with PullStream?

This is what I have:

client, err := signalr.NewClient(ctx, conn)
if err != nil {
    return c, err
}

if err := client.Start(); err != nil {
    return c, err
}

if err := <-client.Send("SubscribeWithCurrentState", c.charger, true); err != nil {
    return c, err
}

go func() {
    productUpdateChan := client.PullStream("ProductUpdate")
    for res := range productUpdateChan {
        if res.Error != nil {
            c.log.ERROR.Println("ProductUpdate", res.Error)
        } else {
            c.log.TRACE.Printf("ProductUpdate %s", res.Value)
        }
    }
}()

Unfortunately, this always errors:

class=Client connection=Dbd8yRimrV-ORMlmZj6SLQ level=debug caller=loop.go:186 event="message received" message="signalr.invocationMessage{Type:1, Target:\"ProductUpdate\", InvocationID:\"\", Arguments:[]interface {}{json.RawMessage{0x7b, 0x22, 0x6d, 0x69, 0x64, 0x22, 0x3a, 0x22, 0x45, 0x48, 0x36, 0x33, 0x35, 0x38, 0x37, 0x30, 0x22, 0x2c, 0x22, 0x64, 0x61, 0x74, 0x61, 0x54, 0x79, 0x70, 0x65, 0x22, 0x3a, 0x32, 0x2c, 0x22, 0x69, 0x64, 0x22, 0x3a, 0x31, 0x36, 0x2c, 0x22, 0x74, 0x69, 0x6d, 0x65, 0x73, 0x74, 0x61, 0x6d, 0x70, 0x22, 0x3a, 0x22, 0x32, 0x30, 0x32, 0x31, 0x2d, 0x30, 0x39, 0x2d, 0x31, 0x37, 0x54, 0x30, 0x36, 0x3a, 0x30, 0x36, 0x3a, 0x34, 0x32, 0x5a, 0x22, 0x2c, 0x22, 0x76, 0x61, 0x6c, 0x75, 0x65, 0x22, 0x3a, 0x22, 0x31, 0x22, 0x7d}}, StreamIds:[]string(nil)}"
class=Client connection=Dbd8yRimrV-ORMlmZj6SLQ level=info event=getMethod error="missing method" name=ProductUpdate reaction="send completion with error"

It seems I have no way getting around the missing method error?

andig commented 3 years ago

I also have the suspicion, that the code might be racy: if the server sends data before the client.PullStream has been created these will error. Creating the pull stream before starting the client however panics.

andig commented 3 years ago

Oh, got it. Shouldn't use streams put public methods.

philippseith commented 3 years ago

PullStream pulls a stream from the server. It sends a StreamingInvocation to the server. Maybe the name is missleading. See https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/docs/specs/HubProtocol.md#streaming. The example server does publish its values by calling methods on the server (callbacks, therefore the javascript client is has this 'on' method). But as I wrote here https://github.com/philippseith/signalr/pull/63#issuecomment-922270306, calling methods on the server and the client is barely the same thing, so I decided to implement calling callbacks like calling hub methods, aka as calling public methods of the hub (server) or receiver (client)

philippseith commented 3 years ago

FYI streaming has less protocol overhead than calling the client for every new item.

andig commented 3 years ago

FYI streaming has less protocol overhead than calling the client for every new item.

Not my choice as client…

As I‘m slowly beginning to understand how signalR actually works, I‘m not entirely happy with the API exposed. The extensive use of reflection make it hard to type-check anything at compile time. Exposing the handlers as public methods on the receiver is not self-explanatory. If you should ever consider a 2.0 version it might be nice to make the handlers explicitly configurable per function, maybe something like

f := func(param …interface{}) { … }
party.Handler(„ProductUpdate“, f)
philippseith commented 3 years ago

IMHO, type checking is not missing because of the usage of reflection, but because SignalR has no IDL like gPRC or https://github.com/twitchtv/twirp. Reflection is used by the archetypical .net SignalR server and has been brought into the go server by David Fowler himself when he started this project. But you are right: Adding a handler interface is a simple (and simply understood) alternative to the current strategy.