[hermes] Add WS #773
[hermes] Add WS #773
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
| pub struct Subscriber { | ||
| id: SubscriberId, | ||
| closed: bool, | ||
| store: Store, |
There was a problem hiding this comment.
This kind of dependency is not great. We should rethink about the structure in the coming refactors.
There was a problem hiding this comment.
this is just saying that each websocket subscriber holds a reference to the same global store of price information? if so, that seems ok to me?
There was a problem hiding this comment.
actually does this store need a lock to manage writes / reads of a feed across threads?
There was a problem hiding this comment.
There are locks to prevent misuse (Rust doesn't allow otherwise :D) but I also created the direction of dependency in a way that it doesn't introduce a deadlock.
There was a problem hiding this comment.
I think generally it's fine. I don't like it much because it's half actor and half part of a global component.
I could store the Subscriber as a global component and let the global dispatcher directly call the methods and I tried. The result wasn't great because the Subscriber has a stream coming from the client and when it gets closed on destruction it should remove itself from the global reference to it (which it has access to the reference) and ...
I think the actor model offers better decoupling but that's a bigger design decision that I didn't want to make on this PR.
jayantk
left a comment
jayantk
left a comment
There was a problem hiding this comment.
I don't totally understand how this works but it seems generally reasonable. I think some tests would be nice (though understand why you wouldn't want to do that given you're about to refactor everything)
| pub struct Subscriber { | ||
| id: SubscriberId, | ||
| closed: bool, | ||
| store: Store, |
There was a problem hiding this comment.
this is just saying that each websocket subscriber holds a reference to the same global store of price information? if so, that seems ok to me?
| pub struct Subscriber { | ||
| id: SubscriberId, | ||
| closed: bool, | ||
| store: Store, |
There was a problem hiding this comment.
actually does this store need a lock to manage writes / reads of a feed across threads?
Co-authored-by: Reisen <[email protected]>
This PR adds Websocket implementation with price service interface to Hermes. Each websocket connection is wrapped in a Subscriber struct which acts as an actor.
As discussed before, after merging this I am planning to restructure everything to only support accumulator and things gets changed. The current structure is not great and will be changed when the refactor is done.