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

Add support for mutable access #41

Closed
wants to merge 8 commits into from
Closed

Add support for mutable access #41

wants to merge 8 commits into from

Conversation

hawkw
Copy link
Owner

@hawkw hawkw commented Apr 6, 2020

This is still quite rough, needs docs, additional testing, and cleanup, but it basically works.

Signed-off-by: Eliza Weisman eliza@buoyant.io

Copy link
Collaborator

@bIgBV bIgBV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Something to note is the number of guards we are exposing in the public API.

src/lib.rs Show resolved Hide resolved
}
}

pub fn is_nonexistent(&self) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as above.

@bIgBV
Copy link
Collaborator

bIgBV commented Apr 17, 2020

Having gone through the changes I think you got the main parts all done. I've got one thought though. I think we can provide more granular error messages here. Now we have multiple failure conditions for why a get call fails:

  • Shard is full
  • Someone else has already removed the value present in that slot (generation has updated)
  • Someone else is has exclusive access to it.

As a user, I'd like to know which of these three cases caused the failure. I see that you already created a user facing error type GetMutError with a couple of errors, but I think we need to change methods on Slot to return a Result<T, SlotError> instead of an Option.

EDIT: Or we can rename GetMutError to StorageError and extend that to include the other cases and use it for all get* methods.

@bIgBV
Copy link
Collaborator

bIgBV commented Apr 17, 2020

Besides that, I think it's mostly just tests and docs.

@bIgBV bIgBV marked this pull request as ready for review May 10, 2020 21:43
@bIgBV
Copy link
Collaborator

bIgBV commented May 10, 2020

@hawkw Added tests for the slab impl and looks like you had already created tests for the pool impl. Also added documentation. I figured I could port the create API to return a get_mut guard in a separate PR.

@bIgBV bIgBV changed the title [WIP] add support for mutable access Add support for mutable access May 10, 2020
src/lib.rs Outdated Show resolved Hide resolved
hawkw and others added 6 commits October 15, 2020 13:21
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: Bhargav <bIgBV@users.noreply.github.com>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw closed this in #48 Oct 19, 2020
hawkw added a commit that referenced this pull request Oct 19, 2020
This PR changes the `Pool::create` method to return a `RefMut` guard,
which allows mutable access to the new pooled item, rather than taking a
closure that is used to initialize the item. A new method,
`Pool::create_with`, provides the old behavior.

When a slot is referenced by a `RefMut` guard, it may not be accessed by
other threads. Calls to `Pool::get` with that value's key will return
`None` while a `RefMut` exists. The `RefMut` guard may be downgraded to
an immutable `Ref` guard, allowing other threads to access the pooled
item while retaining a guard for immutably accessing it.

Closes #41.
Fixes #16.

BREAKING CHANGE:

Changed `Pool::create` to return a `RefMut`, rather than taking
a closure and returning a key.

Renamed `PoolGuard` to `pool::Ref`.
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.

None yet

2 participants