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

What is the use case for BidirectionalStreamer #670

Open
tharvik opened this issue Dec 22, 2020 · 3 comments
Open

What is the use case for BidirectionalStreamer #670

tharvik opened this issue Dec 22, 2020 · 3 comments
Labels

Comments

@tharvik
Copy link
Contributor

tharvik commented Dec 22, 2020

I'm refactoring the streaming capabilities of onet. Now, I'm having issues understanding what is the need for a bidirectional stream. Which real use case cannot be solved by opening a new stream for each client input?

  • to avoid many TCP open, one can use a ClientKeep
  • currently, it is not clear, either in code or in documentation
    • what happen when a service didn't finish streaming and the client sends a new message
      • if theses are sent on the same channel, messages will be mixed
    • should the service wait anyway on the "stop service" signal?
    • should a "stop service" close all the returned channels? isn't theses supposed to be the same?

I was thinking of removing the bidirectionnal support, and having the streaming calls returning a new connection/channel for each.

@ineiti
Copy link
Member

ineiti commented Jan 4, 2021

@nkcr added this for the columbus explorer. I got to admit that I didn't follow very closely the use-case, so it might be possible to optimize it. However, as far as I know, it's in use, so it shouldn't be removed!

@nkcr
Copy link
Contributor

nkcr commented Jan 4, 2021

I am using this functionality for the PaginateBlock streaming handler. Bi-directional streamer should be supported since we are using websocket (if not why using a full-duplex communication protocol ?).

@tharvik
Copy link
Contributor Author

tharvik commented Jan 4, 2021

Bi-directional streamer should be supported since we are using websocket (if not why using a full-duplex communication protocol ?).

Still, I don't quite see an actual use case for that yet.

But we can also support it, and for that, we should give more control of the stream to the service, such as calling it with a recv-channel of messages, not calling it on each message. That way, the service can actually handle multiple message without keeping the state inside the service's struct (and we all know how it goes when using a changing shared state...).

Using a recv-channel for incoming message does make the implementation easier: closing the channel means the client closed the connection, no need for an explicit "service stop channel". That actually solve most of the points I raised.

I am using this functionality for the PaginateBlock streaming handler.

If I'm comparing it with TestServiceProcessor_ProcessClientStreamRequest, it is incorrectly implementing a streaming handler, as returned channels are created each time the function is called but from the test, it should return the same channels for a given client if some exchange is already in progress.

So we agree that at least the documentation/tests should be updated :)

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

No branches or pull requests

3 participants