OrleansContrib / SignalR.Orleans

SignalR backend based on Orleans.
MIT License
295 stars 64 forks source link

Persistence design #91

Closed galvesribeiro closed 5 years ago

galvesribeiro commented 5 years ago

Recently we enabled users to define which storage they want to use to store the User/Group state so it can be stored on a persistent storage. With that, a new problem emerge as explained on #90.

The reason for that problem, is because we had never serialised that state before. It was always stored on InMemory storage provider.

The problem is, the implementation of StreamSubscriptionHandle doesn't have a default ctor. Even thought it is marked as [Serializable], Json.Net can't serialise/deserialise it.

The common sense would say that we should open an issue on Orleans repo and ask for a new ctor there.

However, while investigating the issue, I've figured out that it is pointless to have this storage of the subscription handle if the stream provider is SMS. In our case, if the silo/cluster fail and/or shutdown, then we would have the handle BUT, the messages that the handle points to at the subscription are already gone, so no chance to get back and resume the subscription.

We weren't seeing this issue before because we wasn't serializing the state but now that we have, it makes no sense to have it.

We have three options:

  1. (not ideal) Remove/revert the persistence and tell anyone that all messages are transient and if the silo dies, they die with it. This beats part of the purpose of the backplane when it is compared to other implementations like the Redis one which is persistent by default.
  2. Implement a custom stream provider that is backed by our grains. That way we would store the messages on the grain state among with the subscription handles so the user can restore it once the subscription is resumed.
  3. Redesign the whole provider so it drop the usage of SMS and use grain methods to do the handling of messages. That requires some thoughts, but would be simpler than 2.

@claylaut @stephenlautier what do you think? Am I missing something here?

Thanks!

stephenlautier commented 5 years ago

I believe another simple way of doing it is to keep a simpler state and hydrate from it e.g.:

What we need to make sure and test properly is to test with .ResumeAsync and also calling the .Add as suggested above, to not have multiple subscriptions on the same message

galvesribeiro commented 5 years ago

Okey, good idea. Will experiment on that. Thanks!

alfkonee commented 5 years ago

I believe another simple way of doing it is to keep a simpler state and hydrate from it e.g.:

  • store the ConnectionId's as List<string> (instead of dictionary) (in persisted state)
  • on activation generate the dictionary as is, and keep it as a private field by calling .Add

What we need to make sure and test properly is to test with .ResumeAsync and also calling the .Add as suggested above, to not have multiple subscriptions on the same message

Instead of using a generic Dictionary I think prevent duplication we can use a HashSet instead of list and ConcurrentDictionary instead of Dictionary this will ensure only one record per connection

stephenlautier commented 5 years ago

I believe another simple way of doing it is to keep a simpler state and hydrate from it e.g.:

  • store the ConnectionId's as List<string> (instead of dictionary) (in persisted state)
  • on activation generate the dictionary as is, and keep it as a private field by calling .Add

What we need to make sure and test properly is to test with .ResumeAsync and also calling the .Add as suggested above, to not have multiple subscriptions on the same message

Instead of using a generic Dictionary I think prevent duplication we can use a HashSet instead of list and ConcurrentDictionary instead of Dictionary this will ensure only one record per connection

Yes HashSet should be better, ConcurrentDictionary shouldn't be needed tho, since a Grain is single-threaded