Skip to content

fix: move debug_assert check #275

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

Merged
merged 1 commit into from
Mar 5, 2023
Merged

Conversation

YoniFeng
Copy link
Contributor

@YoniFeng YoniFeng commented Mar 3, 2023

Fix needed after #268
The fix is only for debug_assert correctness. There's no functional effect.
i.e. version 0.8.6 might cause debug tests to fail, but it isn't a (functionally) breaking change.

Explanation
Moving the lock to be per-bucket changed the DYNAMIC_SET's API so that it doesn't need to be locked (i.e. DYANMIC_SET is not wrapped with a Mutex).
The Atom's fn drop changed from

fn drop(&mut self) {
        if self.tag() == DYNAMIC_TAG {
            let entry = self.unsafe_data.get() as *const Entry;
            if unsafe { &*entry }.ref_count.fetch_sub(1, SeqCst) == 1 {
                drop_slow(self)
            }
        }

        // Out of line to guide inlining.
        fn drop_slow<Static>(this: &mut Atom<Static>) {
            DYNAMIC_SET
                .lock()
                .remove(this.unsafe_data.get() as *mut Entry);
        }
    }

to

impl<Static> Drop for Atom<Static> {
    #[inline]
    fn drop(&mut self) {
        if self.tag() == DYNAMIC_TAG {
            let entry = self.unsafe_data.get() as *const Entry;
            if unsafe { &*entry }.ref_count.fetch_sub(1, SeqCst) == 1 {
                drop_slow(self)
            }
        }

        // Out of line to guide inlining.
        fn drop_slow<Static>(this: &mut Atom<Static>) {
            DYNAMIC_SET.remove(this.unsafe_data.get() as *mut Entry);
        }
    }

(the lock() is gone)
Now when we enter DYNAMIC_SET.remove(), there's no longer a lock guarantee.
Until we lock the bucket, another thread could be in the middle of performing a DYNAMIC_SET.insert() for the same string.
Therefore, the debug_assert!(value.ref_count.load(SeqCst) == 0) is premature - it needs to occur after we take the lock for the bucket.

Caught at swc-project/swc#6980 in a failed test.

Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

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

Thanks for diagnosing this!

@jdm
Copy link
Member

jdm commented Mar 5, 2023

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 120ba6c has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 120ba6c with merge 7f3789b...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-github
Approved by: jdm
Pushing 7f3789b to master...

@bors-servo bors-servo merged commit 7f3789b into servo:master Mar 5, 2023
@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 6, 2023

@jdm hate to have to ask again.. but could you please bump another version?

hopefully this is the last activity on this crate for a long time :)

@jdm
Copy link
Member

jdm commented Mar 7, 2023

Done!

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.

3 participants