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

keep connected && transport error handler #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cravler
Copy link

@cravler cravler commented Sep 17, 2018

package main

import "fmt"
import "github.com/thesyncim/faye"
import "github.com/thesyncim/faye/subscription"

func main() {
    f, err := fayec.NewClient("ws://127.0.0.1:8000/faye")
    if err != nil {
        panic(err)
    }

    f.OnTransportError(func(err error) {
        panic(err)
    })

    var sub *subscription.Subscription
    sub, err = f.Subscribe("/messages")
    if err != nil {
        panic(err)
    }
    err = sub.OnMessage(func(data message.Data) {
        fmt.Println(data)
    })
    if err != nil {
        panic(err)
    }
}

@thesyncim
Copy link
Owner

thesyncim commented Sep 19, 2018

Thanks for your interest in this project.

I'am Ok to extend the client interface with OnTransportError(this should be pretty useful in mobile devices).
I'm Not OK to panic in case on error , check my comments.

Also, be aware that i'm probably going to break some API like OnMessage(func(data message.Data) to OnMessage(onMsg func(chanel string,data message.Data) in order to support wildcard subscriptions.

thank you!

@@ -60,17 +62,17 @@ func (w *Websocket) Init(endpoint string, options *transport.Options) error {
return nil
}

func (w *Websocket) readWorker() error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep the current function signature

for {
select {
case err := <-w.stopCh:
return err
panic(err)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not panic in user libraries, I'm sure you don't want to panic when you disconnect the client (see w.stopCh)

default:
}
var payload []message.Message
err := w.conn.ReadJSON(&payload)
if err != nil {
return err
panic(err)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not panic if the read fail, FAYE protocol should handle auto-reconnect( this is going to be handled in the upcoming commits )

@@ -83,6 +85,16 @@ func (w *Websocket) readWorker() error {
if transport.IsMetaMessage(msg) {
//handle it
switch msg.Channel {
case transport.MetaConnect:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is going to be handled in the upcoming commits as well, we shouldn't reconnect always.
If the server sends advise.reconnect==none we should stop

@cravler
Copy link
Author

cravler commented Sep 20, 2018

I understood that work in progress and there are might be breaking changes, but thanks for explanations.

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

Successfully merging this pull request may close these issues.

2 participants