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

Readonly channels and TTL #256

Open
sarge opened this issue Jun 24, 2019 · 5 comments
Open

Readonly channels and TTL #256

sarge opened this issue Jun 24, 2019 · 5 comments
Labels

Comments

@sarge
Copy link
Contributor

sarge commented Jun 24, 2019

Hi there,

If I have a readonly channel, the TTL does not appear to apply to it. Clients can read indefinitely.

Bug or a feature?

Cheers

@Florimond
Copy link
Member

Hello,

I need to explore this further, but so far I would vote for "bug". So, avoid relying on this behavior. I'll have a deeper look as soon as I can.

@sarge
Copy link
Contributor Author

sarge commented Jun 27, 2019

Hi Florimond,

I have dug a little deeper, I followed 2 ideas of how this could have been implemented to see if this is a missing feature or bug in an existing implementation.
If a subscription has a TTL, then something would need to store the TTL and check for invalidations. I had a look at the subtrie implementation.
https://github.com/emitter-io/emitter/blob/master/internal/message/subtrie.go#L65
I can't see where the TTL is stored.

The second point of inquiry was around permissions checking, we can see that this happens when a new subscription is received.
https://github.com/emitter-io/emitter/blob/master/internal/broker/handlers.go#L63
I can see the permission check contract, key, allowed := c.authorize(channel, security.AllowRead)

security.AllowRead only happens in a few places which is nice but none of them run over the list of available subscriptions.

My conclusion is that there is no code that currently deals with a subscription expiring.

I am interested in helping with this issue.

I would suggest that we store the, expiry time when the subscription is created. A go routine loops over the subtrie and when an expired subscription is found:
a) evict entries that have expired?
b) evict entries that have expired and notify the client?
c) close the socket - forcing the client to reconnect and resubscribe (we could just store the earliest expiry on the connection if this is the case)

Interested in your thoughts, happy to be corrected.

Cheers

@kelindar
Copy link
Contributor

We'd need to think about this a bit more. Essentially I see couple of things that need to be done:

  1. Each subscription gossiped would need to carry the expiration time.
  2. During lookup (?) we could introduce a branch which checks whether current time < expiration time of the subscription, and potentially evict them.

Not sure about closing the socket. We could send an error message down to the client which would tell that the subscription is expired and let the client do something about it.

@postacik
Copy link

Is this an issue for all versions or just the latest commits?

@sarge
Copy link
Contributor Author

sarge commented Jun 29, 2019

What is the implication on the client of receiving the unsubscribed message? They would need to re-authenticate and resubscribe. There is the potential for some lost messages during that time between the unsubscription and the resubscription.

What durability guarantees can we provide during that time?

What would we support for the in memory persistence with zero message retention? Which I think is the trickiest case?

A suggestion is either
a) close the socket - a bit heavy handed, but exisiting applications should already handle reconnections, so backwards compatible? Does not provide extra resiliency over the existing expectations of a stable web socket connection. I.e. you could lose an update.
b) expire the subscription as suggested, sending a new error message to clients before the unsubscription. Allows clients to resubscribe before the expiry. 30 seconds to resubscribe?
[Edit]This also comes with the problem of how many error messages to send - one per active node?

I like the eviction on publish. No go routines needed. I do worry about lost messages though.

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

4 participants