-
Notifications
You must be signed in to change notification settings - Fork 113
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
Delete public links on GET #1364
Delete public links on GET #1364
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
@refs we can easily add a goroutine to the json manager to clean the links without having to do it on these ops? |
and |
@refs when the driver is started as part of the init process for example. |
The only drawback I see with it is the number of unnecessary cycles that routine would do just checking whether a share is expired or not, then since we're polling the db, that also needs to work with the locks, I see that even using Now even if we got the goroutine blocked polling the json "db" for expired shares there could still be the case that a share is expired the moment it's accessed or listed, this is the argument that drove me to go with this "delete on-demand" approach. Let me spend some time using time.AfterFunc and see how far I get |
@labkode have a look at The abstraction was done due to the There is no way to provide a logger since the manager has no awareness of service configuration, so at the moment the I have not measured the performance impact of this nor with a small sample db or a large one, so the costs of locking, serializing, iterating and removing are not being accounted for, only functionality. |
A small remark to add is that even with this janitor logic I'd still like to leave the on-demand checks on each method to avoid stale data. |
ping @labkode |
Co-authored-by: Alex Unger <[email protected]>
Following up on #1361
Due to tunnel vision and solving one single problem, public links get deleted now on
ListPublicLinks
operations, but as long as we don't have background jobs they MUST also be deleted onGetPublicLink
operations.There is the scenario that a public link is expired but
ListPublicLink
has not run, accessing a technically expired public link is still possible.A proposed solution is deleting the link on access.