Add support for shared websocket messages #104
Conversation
This adds the `SharedMessage` enum as well as the `write_shared_message` for both `Websocket` and the `WebsocketContext`.
|
This change could be propagated to |
There was a problem hiding this comment.
While I'm not against the proposed changes, I think that it may make the usage of the library a little bit more complicated. It might be not obvious for the user why do we have 2 different functions which have the same semantics, but slightly different input data types.
Provided that Bytes is quite popular nowadays, I think it may make sense to use Bytes instead of Vec<u8> by default. It should not affect the performance much.
Alternatively, we could use the Either* and Payload approach inside our regular Message structure, so that the Message::Binary accepts some Payload type (and we can provideFrom implementation to support Bytes and Vec<u8>). I think it would make more sense since it does not change the overall structure of enums much (although it's a breaking change) and it does not introduce an additional function in our high-level API. It also makes less effort for us to propagate the changes in tokio-tungstenite.
@agalakhov , what's your opinion?
UPD. @agalakhov ping :)
|
How about |
|
@agalakhov , this makes sense. I did not write a benchmark for |
|
Hi, is there anything I can do to help with resolving this PR? I need this to efficiently broadcast the same message to multiple clients. |
|
Hi @nikarh , there has been a while since I checked this PR, but it seems that we changed the code quite a bit, so there are some conflicts. I may assume that starting it from scratch would be simpler. IMHO the best way to support this feature would be implementing the approach that @agalakhov suggested. P.S.: In case you come up with a PR, I'll be able to check it once I'm back to the office after the Christmas holidays, so sorry for possible delays in advance :) |
daniel-abramov
changed the title
|
It looks like this PR is quite stale and the amount of conflicts suggests that perhaps writing it from scratch would be simpler than adjusting the code in the PR (just because we touched many similar places in the Thanks everyone for collaboration and discussions around this topic! |
This adds the
SharedMessageenum as well as thewrite_shared_messagefor both
Websocketand theWebsocketContext.This fixes #96.
Essentially I tried to minimize the breaking changes of this PR since its kind of a edge case. However the
Payloadenum might had some additional overhead (although probably minimal) by adding branches.Currently the only breaking change is in the
Error::SendQueueFull(EitherMessage)instead ofError::SendQueueFull(Message). We could however add an error variant to circumvent that (i.e Error::SharedSendQueueFull).