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

feat: add withCapacity function #335

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Jan 5, 2025

Summary

This is an alternative implementation of withSemaphore introduced by #331.

It's different in a few notable ways:

  • Its name is withCapacity (on the fence about this, but I think it's less jargon-y than withSemaphore and that's probably a good thing)
  • It's about 1/3 the size in bytes.
  • Different methods
    • No manual acquire/release and I'm leaning toward that being a good thing, though I want to be sure I'm not overlooking an important use case (maybe @hugo082 can clarify this). Side note: Perhaps, in the future, we should consider adding “lower level” (i.e. more error prone, yet more control) variants as class implementations, which might be where the acquire/release paradigm feels more appropriate.
    • Instead of getRunning(): number, there's a hasCapacity(weight?: number): boolean method. I'm not sure what the use case is for getting the current number of locks, so I'm hoping @hugo082 can clarify.

withMutex

This PR doesn't have a withMutex function, and I'm still on the fence about it being a good addition, since it's easy enough to use withCapacity(1, …) for the same behavior? More clarity on this would be great.

Related issue, if any:

#331

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed
  • Related benchmarks have been added or updated, if needed
  • Release notes in next-minor.md or next-major.md have been added, if needed

Does this PR introduce a breaking change?

No

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

Successfully merging this pull request may close these issues.

1 participant