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

Fix RefMutContainer drop bug #38

Merged
merged 5 commits into from
Jul 2, 2023
Merged

Fix RefMutContainer drop bug #38

merged 5 commits into from
Jul 2, 2023

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Jun 29, 2023

Existing RefMutContainer code also causes undefined behavior easily due to using unsafe code wrong.

For example,

>>> import rraft
>>> c = rraft.Config()
>>> r = c.make_ref()
>>> c = None
>>> r = None
(REPL spinning, not return)

After this patch,

>>> import rraft
>>> c = rraft.Config()
>>> r = c.make_ref()
>>> c = None
>>> r
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
rraft.DestroyedRefUsedError: Cannot use a destroyed object's reference!
>>> c = rraft.Config()
>>> r = c.make_ref()
>>> c = None
>>> r = None
>>> c
>>> r
>>> 

@jopemachine
Copy link
Member Author

I'm not sure making shortcut type like below would be better here.

type RefTable<T> = HashMap<u64, Weak<Mutex<Option<NonNull<T>>>>>;

@jopemachine jopemachine added bug Something isn't working important labels Jun 29, 2023
@jopemachine jopemachine changed the title PoC: Fix RefMutContainer drop bug Fix RefMutContainer drop bug Jun 29, 2023
@jopemachine jopemachine marked this pull request as ready for review June 29, 2023 05:23
@jopemachine
Copy link
Member Author

clippy said like below, so I decided to make an intermediate type like RefTable<T> mentioned before.

warning: very complex type used. Consider factoring parts into `type` definitions
  --> src/utils/reference.rs:16:11
   |
16 |     refs: Arc<Mutex<HashMap<u64, Weak<Mutex<Option<NonNull<T>>>>>>>,
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
   = note: `#[warn(clippy::type_complexity)]` on by default

@jopemachine jopemachine merged commit 9aad47c into main Jul 2, 2023
2 checks passed
@jopemachine jopemachine deleted the fix-retmutcontainer branch July 2, 2023 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working important
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant