Skip to content
This repository has been archived by the owner on Dec 18, 2024. It is now read-only.

INFO-3 Subscription channels remain open after token expiration #751

Closed
wants to merge 2 commits into from

Conversation

lukasmittag
Copy link
Contributor

This fixes that a subscription channel stays open after the token of the subscriber has expired. We need to decide if we want to fix this. Currently it is only fixed for ChangeSubscription which the new API uses. If we want to do it for the old query subscription as well it can be done similiar. To test it take a jwt token of kuksa-val/jwt/ and use a timestamp that expires in the near future -> go to https://www.epochconverter.com/ for timestamps ;)

It is not that pretty in code I think so do not now if we want to fix this in this way/at all. One observation I made was that even if the channel is kept open somehow if you update the token new updates do not come in in the databroker-cli again. Haven't figured out why yet.

@lukasmittag lukasmittag requested review from argerus and erikbosch March 8, 2024 09:56
@lukasmittag
Copy link
Contributor Author

and I opted for closing if a new notification would be sent out so no general repeated check. This means if you do not update soem things channel can stay open a while.

@rafaeling
Copy link
Contributor

Code looks fine to me, tested with python sdk and the channel closes correclty

erikbosch
erikbosch previously approved these changes Mar 12, 2024
Copy link
Contributor

@erikbosch erikbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok for me. But I did not really understood one of the comment, see below. Is that one still valid, or do you think the code is pretty enough (kenough) now?

"It is not that pretty in code I think so do not now if we want to fix this in this way/at all."

@lukasmittag
Copy link
Contributor Author

I just thought that it's maybe not the ideal way code wise. But since @argerus had nothing to complain yet :) From my side we can merge.

@boschglobal boschglobal closed this by deleting the head repository Mar 21, 2024
Copy link
Contributor

@argerus argerus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be a lot easier to treat expired permissions the same way we treat channels that have been closed:

e.g. add a check to Subscriptions::cleanup() that will remove all subscriptions with expired permissions. This will be triggered both periodically, and whenever a SubscriptionError is returned when when calling notify.

kuksa_databroker/databroker/src/broker.rs Outdated Show resolved Hide resolved
@erikbosch
Copy link
Contributor

Databroker has been migrated to https://github.com/eclipse-kuksa/kuksa-databroker.
Please open a new pull request in that repo.

@lukasmittag lukasmittag closed this May 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants