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

Better API for Watch / SSE Streaming #2

Open
ereyes01 opened this issue Nov 4, 2015 · 3 comments
Open

Better API for Watch / SSE Streaming #2

ereyes01 opened this issue Nov 4, 2015 · 3 comments

Comments

@ereyes01
Copy link
Owner

ereyes01 commented Nov 4, 2015

Currently Watch accepts a stop channel and returns an event channel. I'd like to make a simpler API that emulates bufio.Scanner. This should wrap the existing implementation to preserve binary compatibility, and to not break users who might need to select on multiple watchers.

Below is an example of what the new API would look like:

Initialize a watcher, and clean it up when we're done with it:

watcher, err := client.NewWatcher(unmarshaller)
if err != nil {
    // handle error
}
defer watcher.Close()

Iterate through events, and handle errors:

for watcher.Listen() {
    if err := watcher.ParseError(); err != nil {
        log.Println("Malformed event: ", err)
    }

    event := watcher.Event()
    // process event
}

if err := watcher.Error(); err != nil {
    // handle error
}
@ereyes01
Copy link
Owner Author

I might have talked myself out of implementing this feature after trying to get started on it. In the end, I think it will not make streaming/watching Firebase resources significantly easier. The examples above are misleading, as ParseError() and Error() have to be checked on each iteration of Listen(), just like you do with the channel. Receiving either kind of error does not imply the channel has closed.

Stuffing everything into a StreamEvent struct spit out from a channel may seem unwieldy, but it's the simplest way I know to accurately model all the possibilities Firebase can throw at you when streaming changes to a resource.

@ereyes01 ereyes01 reopened this Feb 5, 2016
@ereyes01
Copy link
Owner Author

ereyes01 commented Feb 5, 2016

https://groups.google.com/forum/#!topic/firebase-talk/TRO_bEcWhII

@inlined proposed a nice way to design this by making the Watcher object contain a json.Decoder object. This would get rid of the whole EventUnmarshaller business and allows json parse errors to be handled outside of the library, instead of exposing it via the UnmarshallerError.

I'm thinking through the possible error + disconnection scenarios to make sure that the design accounts for all the possibilities, which are (as far as I know):

In particular, the cancel error is not necessarily a fatal error. Permissions can change to de-authorize you from reading a path, but could also change later to re-authorize you, thus conceivably allowing you to reuse your connection (firebase doesn't seem to have a uncanceled event though). I can't think of a way you could recover from auth_revoked at the moment.

Let's operate on the assumption that a cancel event is a fatal error, which is probably true in most practical cases.

I think another important part we have to account for is to allow the caller to see the event type and the path. These two pieces of information might be necessary to properly decode the event's data payload.

With those things in mind, here's my swing at how this API could look like:

watcher, err := client.Child("some/place").NewWatcher()
if err != nil {
    // handle error
}
defer watcher.Stop()

var (
    streamErr error
    foo Foo
    event firebase.WatchEvent
)

for {
    event, streamErr = watcher.NextEvent()
    if streamErr != nil {
        break
    }

    // event.Type is either "put" or "patch"
    // event.Path has the path that this event is for  

    if err := event.DecodeData(&foo); err != nil {
        log.Println("Couldn't parse event:", err)
        continue
    }

    // do something with foo
}

log.Println("Finished with err ==", streamErr)

When I get some time, I'll revisit this and hammer this out. In the meantime, if anyone has feedback, please feel free to provide it.

Thanks, Eddy

@ereyes01
Copy link
Owner Author

As illustrated in the discussion in #9, event.Type and event.Path are important to determining what you can successfully unmarshal into an object via the DecodeData method.

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

No branches or pull requests

1 participant