juju / mutex

Provides a named machine level mutex shareable between processes.
Other
28 stars 11 forks source link

Allow arbitrary names #16

Open jotadrilo opened 1 year ago

jotadrilo commented 1 year ago

This PR aims to fix https://github.com/juju/mutex/issues/15. It moves restrictions to the actual implementation, so the caller does not have to deal with them.

In this case, the validations were about having "expected" strings (they have to match a pattern) and not being more than 40 chars long.

These restrictions can be bypassed by using a hash algorithm.

Hence, this PR re-implements the acquire logic to use the SHA1 checksum as the mutex name, which meets all the expectations.

EDIT: I first tried using the SHA1 algorithm. But the resulting checksum might start with a number. Is starting with a letter a mandatory constraint?

EDIT 2: I secondly tried using the MD5 algorithm. The resulting checksum is 32 chars long so it is OK, even though it might not start with a letter. I have added support for a prefix with a max length of 7 charts, starting with a letter. The default prefix will be mutex.


Questions

Why the mutex name must start with a letter?

If this is not a constraint, we can use SHA1 algorithm for simplicity.

Why the mutex name max length must be 40 chart length?

If this is not a constraint, we can support using arbitrary prefixes regardless the hash algorithm used to compute the mutex name.

jujubot commented 1 year ago

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

jujubot commented 1 year ago

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

jotadrilo commented 1 year ago

@hpidcock @anvial would you mind taking a look at these changes? I am happy to look into a different alternative if you believe there are better ones.