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

Improve StreamingHandlers contract #675

Open
ineiti opened this issue Mar 23, 2021 · 3 comments
Open

Improve StreamingHandlers contract #675

ineiti opened this issue Mar 23, 2021 · 3 comments

Comments

@ineiti
Copy link
Member

ineiti commented Mar 23, 2021

The current RegisterStreamingHandlers is not ideal when it comes to implement a streaming service:

  • every new client message calls the endpoint again
  • the endpoint cannot return an error once the streaming started
  • the contract between onet and the endpoint is not clear

A first, simplified method signature for a blocking endpoint:
Endpoint(<-input, ->output) error

  • arguments:
    • input-channel of messages from client
    • output-channel of messages from endpoint
  • return:
    • error, which can be nil to indicate the endpoint finished its work and is done

Onet would do the following:

  • setup the input- and output-channel
  • call the endpoint for each new connection from a client using Client.Stream
    • the endpoint is blocking and listening on the input channel for new messages, and sending messages on the output channel
    • if the endpoint is done, it can return with a nil error to indicate successful termination
    • any non-nil error indicates an error by the endpoint
  • send every message from the client through the input channel to the endpoint
  • send every message from the output-channel of the endpoint to the client
  • close the input-channel if the websocket is closed by the client
    • all further messages in the output-channel are ignored
    • any returned error from the endpoint is ignored
    • who should close the output-channel?
  • close the connection if the endpoint returns
    • if the error is nil, the connection is closed without an error
    • if the error is not nil, the error is sent as the reason for closing the connection

Comments, @tharvik @cgrigis @nkcr ?

@tharvik
Copy link
Contributor

tharvik commented Mar 25, 2021

Great idea, it's a bit hard to use currently. I'm adding some comments below.

A first, simplified change could be:

smth like Handler(inputs <-chan, outputs chan->, errors chan->)?
I think that using smth like Handler(inputs <-chan, outputs chan->) error would be more idiomatic, so that one can use the classic if err != nil of golang without having to send something on the channel and then returning. Also, as zero-or-one message is expected on the error channel, no need to use a channel, nil and != nil has the same meaning.

  • call the endpoint only once for each connection from a client

The client uses Client.Stream to call a streaming handler, is that what you mean by "each connection"?

  • one of the following actions triggers onet to close the input-channel:

What happens if the handler finishes and the output channel isn't closed? (discard if you're thinking of Handler(<-chan) (chan->, chan->, error), which I personally don't like)

Some other remarks:

  • onet needs to take care of all channel creation/closing, as to avoid missuses
  • send/recv only channels are cool, and safer
  • StreamingConn.ReadMessage is quite cumbersome to use, using a classic channel on the client might be easier

@ineiti
Copy link
Member Author

ineiti commented Mar 26, 2021

  • endpoint behaviour: when onet calls the endpoint, should it directly return (my first idea), or use an endless loop and only return if it's done (what I gather from your comment)? Probably the second one is better, so your comments about the error-channel make more sense.
  • Error-channel: if the endpoint is blocking, your comment absolutely makes sense, and we don't need this channel
  • endpoint signature: if the endpoint is blocking, this discussion is moot, and the only signature that makes sense is Handler(<-input, ->output) error

I'm updating the description

@ineiti
Copy link
Member Author

ineiti commented Mar 26, 2021

To understand @tharvik's comments, here the original description:

The current RegisterStreamingHandlers is not ideal when it comes to implement a streaming service:

  • every new client message calls the endpoint again
  • the endpoint cannot return an error once the streaming started
  • the contract between onet and the endpoint is not clear

A first, simplified change could be:

  • input:
    • input-channel of messages from client
  • output:
    • output-channel of messages from endpoint
    • error-channel from endpoint

Onet would do the following:

  • call the endpoint only once for each connection from a client
  • send every message from a client through the input channel
  • send every message from the output-channel to the client
  • send the first error from the endpoint to the client
  • one of the following actions triggers onet to close the input-channel:
    • the websocket is closed by the client
    • the output-channel closes, plus onet will close the websocket
    • an error is received through the error-channel, plus onet will close the websocket with the given error

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

No branches or pull requests

2 participants