assertion failure: "participant was expected to be unpinned"

[joshlf]

I'm running into an assertion failure on this line, "participant was expected to be unpinned." Is that error possible to induce via incorrect use of Crossbeam, or does it represent a bug in Crossbeam? If it's the latter, I can provide more details.

[ghost]

Looks like a bug. Can we reproduce somehow?

[joshlf]

Yep, check out this commit, cd into the elfmalloc directory, and run cargo test obj_alloc_many_pages_many_threads_u24. The error is nondeterministic, so you may have to try multiple times before it appears. I know the code is pretty complicated, so feel free to ask me any questions about it. Briefly, the elfmalloc code uses the BagPipe data structure from the bagpipe crate (in /bagpipe), and that's the thing that uses Crossbeam.

[ghost]

I believe the problem is that one Handle is being shared among multiple threads.

Note that cloning a handle simply gives you another reference to the same one. Cloning does not register a brand new handle. Perhaps that's the main source of confusion here.

Bug fix: ezrosent/allocators-rs@a6f64c4...stjepang:handle-bugfix

I wonder if we could do something to prevent mistakes like this one in the future. Can we somehow improve the Collector/Handle/Guard interface?

[Thomasdezeeuw]

Would #70 prevent this?

[ghost]

@Thomasdezeeuw Unfortunately no, since they're manually implementing Send and Sync for their data structures and overriding our !Send and !Sync bounds.

[Amanieu]

I think the name Handle is misleading since it implies that it is a general handle for the collector. Maybe we should rename it to LocalHandle?

[Thomasdezeeuw]

@stjepang Ah, I see. However now Handle isn't Send that isn't valid anymore. In the old implementation, where Handle was Send, the manual implementation was correct (although not really).

@Amanieu I think changing it to LocalHandle would be a good improvement.

[joshlf]

Ah, I see. My bad.

Is there a reason that Handle::clone can't register a new handle? I don't imagine many people would want to make a new Handle just to also use in the same thread. Maybe there could be a shallow_clone method added for this purpose if you're concerned that it's a valid use case?

[ghost]

I'm not sure what to do here. Clearly, Handle::clone can be easily misunderstood and lead to subtle bugs, so we should do something.

While the current behavior of clone could be useful in theory, I'm not aware of any practical uses yet (only misuses :)). Maybe we should simply delete the Clone implementation for Handle?

@joshlf I'm curious: cloning a Collector has similar behavior in that it creates another reference to the same underlying GC. Did you ever find Collector::clone useful? Do you find it potentially confusing, too?

Would perhaps a different naming scheme help? Maybe we should communicate better than Collector, Handle, and Guard are all in fact just pointers to the same thing, and cloning creates another one of the same kind.

[Amanieu]

I agree that Handle::clone isn't very useful, and we could probably do without it.

[joshlf]

Honestly, I don't find the Collector/Handle distinction very useful. For my use cases, what I need is to be able to create a handle to a new collector and then to be able to create new handles to be used by other handles on my data structure. In other words, the following API would be sufficient for me:

impl Handle {
    /// Create a new garbage collector and return a handle to it.
    fn new() -> Handle;
    fn pin(&self) -> Guard;
    fn is_pinned(&self) -> bool;
}

impl Clone for Handle {
    fn clone(&self) -> Handle; // deep copy, registering itself anew with the underlying collector
}

Now, there's an argument to be made for needing to store a collector or handle in a place that multiple threads can access it in order to create new handles for themselves. In that case, I would imagine it would be sufficient to make Handle Sync and make pin take a &mut self.

[joshlf]

Put another way, I don't think it's ever useful to need to have the distinction "this is the real collector, and these other things are just references to it" be visible to the user.

[Amanieu]

The distinction is useful if you are checking that all Guards used with a certain data structure all belong to the same Collector. See crossbeam-rs/crossbeam-skiplist#4.

[joshlf]

How about adding a Handle::is_member(&self, g: &Guard) -> bool method?

[Amanieu]

Note that each data structure needs to hold a reference to a Collector. This is pretty cheap, since it only increments the reference count of the Global. A Local does not need to be created.

Using a Handle as you propose actually has a much bigger hidden cost. Every PINNINGS_BETWEEN_COLLECT (128) times Guard::pin is called, there is a check to see if the epoch can be advanced. This requires iterating over all Locals that are registered with the Global and checking their associated epoch counter. Therefore you want to keep the number of Handles as low as possible, ideally only one per thread.

[joshlf]

Ah, gotcha.

[jeehoonkang]

I'd like to propose we discuss the Handle API in #73.