tus / tusd

Reference server implementation in Go of tus: the open protocol for resumable file uploads
https://tus.github.io/tusd
MIT License
2.92k stars 465 forks source link

Make use of locker mandatory #1119

Open Acconut opened 2 months ago

Acconut commented 2 months ago

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 https://github.com/tus/tusd/issues/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.

Murderlon commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

Murderlon commented 2 months ago

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.