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

Make use of locker mandatory #1119

Open
Acconut opened this issue Apr 23, 2024 · 5 comments
Open

Make use of locker mandatory #1119

Acconut opened this issue Apr 23, 2024 · 5 comments
Labels
breaking change Backwards compatibility breaking change

Comments

@Acconut
Copy link
Member

Acconut commented Apr 23, 2024

Currently, a composer can be created without specifying a lock provider. Such setups should only be used for testing purposes but not for production as explained in https://tus.github.io/tusd/advanced-topics/locks/.

We should make the use of a locker mandatory to make this clear. This is also motivated by questions such as in #1110, where users where unsure whether they need a locker. This change would also allow us to remove some code checking for the existence of a lock provider.

However, this must be done in a major release since this would be a breaking change.

@Acconut Acconut added the breaking change Backwards compatibility breaking change label Apr 23, 2024
@Murderlon
Copy link
Member

Murderlon commented Apr 23, 2024

Why not simply add a default in case it's missing? That's what we do in tus Node.js and it wouldn't be a breaking change.

@Acconut
Copy link
Member Author

Acconut commented Apr 23, 2024

Maybe I have to differentiate a bit more: For users of the tusd CLI we already add lockers by default, so for them nothing would change. For users of the tusd package, we don't set a default storage and also no default locker since they proper implementation (in-memory, file-based, distributed) depends on their use case. We could default to in-memory locks, but might work well in development and then fail in production without users understanding what is going on. I would prefer if we make our users more aware of this decision they have to make, which would be the case if we error out on a missing lock implementation. Of course, all of this must be accompanied by proper documentation.

@Murderlon
Copy link
Member

We could default to in-memory locks, but might work well in development and then fail in production without users understanding what is going on

I would argue the exact opposite, accidentally not using a lock is what causes failures in production without users understanding. With Go I'm much more reluctant for major versions than in JS, where you have better version tooling. So with that in mind I'd say preventing a major version at all cost and defaulting to memory locker is better.

proper implementation (in-memory, file-based, distributed) depends on their use case

If you run a complex distributed setup you are already aware concepts such as lockers need to change. With good docs indicating this, I'd say it's much less likely for an accidental memory locker to cause problems than it is for the majority of people with simpler setups to forget to add a lock if they don't upgrade.

@mitar
Copy link

mitar commented Apr 23, 2024

I think Go made it pretty easy to work with major versions. I would not shy away from major version bumps.

@Murderlon
Copy link
Member

There's a difference between shying away from major versions for the sake of it and considering alternatives which may prevent it without loosing out on quality. Not matter how you put it, major versions are not ideal. And the main argument was this:

If you run a complex distributed setup you are already aware concepts such as lockers need to change. With good docs indicating this, I'd say it's much less likely for an accidental memory locker to cause problems than it is for the majority of people with simpler setups to forget to add a lock if they don't upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Backwards compatibility breaking change
Projects
None yet
Development

No branches or pull requests

3 participants