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

recycler has problems recycling objects in a multi-threaded environment #10986

Closed
xiaoheng1 opened this issue Feb 2, 2021 · 4 comments · Fixed by #11037
Closed

recycler has problems recycling objects in a multi-threaded environment #10986

xiaoheng1 opened this issue Feb 2, 2021 · 4 comments · Fixed by #11037
Milestone

Comments

@xiaoheng1
Copy link
Contributor

Expected behavior

An object is collected in a multi-threaded environment and only exists in one queue.

Actual behavior

An object is collected in multiple queues in a multithreaded environment.

Steps to reproduce

public void testMultipleRecycleAtDifferentThread() throws InterruptedException {
        Recycler<HandledObject> recycler = newRecycler(1024);
        final HandledObject object = recycler.get();
        final AtomicReference<IllegalStateException> exceptionStore = new AtomicReference<IllegalStateException>();

        final CountDownLatch countDownLatch = new CountDownLatch(2);
        final Thread thread1 = new Thread(new Runnable() {
            @Override
            public void run() {
                try{
                    object.recycle();
                }finally {
                    countDownLatch.countDown();
                }
                
            }
        });
        thread1.start();
        //thread1.join();

        final Thread thread2 = new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    object.recycle();
                } catch (IllegalStateException e) {
                    exceptionStore.set(e);
                } finally {
                    countDownLatch.countDown();
                }
            }
        });
        thread2.start();
        //thread2.join();
        
        countDownLatch.await();
        recycler.get();
        IllegalStateException exception = exceptionStore.get();
        if (exception != null) {
            throw exception;
        }
    }

Breakpoint debugging recycler.get() method, you will find that there are two queues in the stack, holding the same object.

Minimal yet complete reproducer code (or URL to code)

Netty version

4.1.59.Final-SNAPSHOT

JVM version (e.g. java -version)

1.8

OS version (e.g. uname -a)

masos

@normanmaurer
Copy link
Member

Hmm... I can't see this. Can you make a screenshot ?

@xiaoheng1
Copy link
Contributor Author

like this.

stack

@normanmaurer
Copy link
Member

@chrisvest can you have a look ?

@chrisvest
Copy link
Contributor

@normanmaurer Yeah, I'll try to spend some time on this.

chrisvest added a commit to chrisvest/netty that referenced this issue Feb 25, 2021
Motivation:
It is possible for two separate threads to race on recycling an object.
If this happens, the object might be added to a WeakOrderQueue when it shouldn't be.
The end result of this is that an object could be acquired multiple times, without a recycle in between.
Effectively, it ends up in circulation twice.

Modification:
We fix this by making the update to the lastRecycledId field of the handle, an atomic state transition.
Only the thread that "wins" the race and succeeds in their state transition will be allowed to recycle the object.
The others will bail out on their recycling.
We use weakCompareAndSet because we only need the atomicity guarantee, and the program order within each thread is sufficient.
Also, spurious failures just means we won't recycle that particular object, which is fine.

Result:
Objects no longer risk circulating twice due to a recycle race.

This fixes netty#10986
chrisvest added a commit that referenced this issue Feb 26, 2021
Motivation:
It is possible for two separate threads to race on recycling an object.
If this happens, the object might be added to a WeakOrderQueue when it shouldn't be.
The end result of this is that an object could be acquired multiple times, without a recycle in between.
Effectively, it ends up in circulation twice.

Modification:
We fix this by making the update to the lastRecycledId field of the handle, an atomic state transition.
Only the thread that "wins" the race and succeeds in their state transition will be allowed to recycle the object.
The others will bail out on their recycling.
We use weakCompareAndSet because we only need the atomicity guarantee, and the program order within each thread is sufficient.
Also, spurious failures just means we won't recycle that particular object, which is fine.

Result:
Objects no longer risk circulating twice due to a recycle race.

This fixes #10986
chrisvest added a commit that referenced this issue Feb 26, 2021
Motivation:
It is possible for two separate threads to race on recycling an object.
If this happens, the object might be added to a WeakOrderQueue when it shouldn't be.
The end result of this is that an object could be acquired multiple times, without a recycle in between.
Effectively, it ends up in circulation twice.

Modification:
We fix this by making the update to the lastRecycledId field of the handle, an atomic state transition.
Only the thread that "wins" the race and succeeds in their state transition will be allowed to recycle the object.
The others will bail out on their recycling.
We use weakCompareAndSet because we only need the atomicity guarantee, and the program order within each thread is sufficient.
Also, spurious failures just means we won't recycle that particular object, which is fine.

Result:
Objects no longer risk circulating twice due to a recycle race.

This fixes #10986
@normanmaurer normanmaurer added this to the 4.1.60.Final milestone Apr 30, 2021
raidyue pushed a commit to raidyue/netty that referenced this issue Jul 8, 2022
Motivation:
It is possible for two separate threads to race on recycling an object.
If this happens, the object might be added to a WeakOrderQueue when it shouldn't be.
The end result of this is that an object could be acquired multiple times, without a recycle in between.
Effectively, it ends up in circulation twice.

Modification:
We fix this by making the update to the lastRecycledId field of the handle, an atomic state transition.
Only the thread that "wins" the race and succeeds in their state transition will be allowed to recycle the object.
The others will bail out on their recycling.
We use weakCompareAndSet because we only need the atomicity guarantee, and the program order within each thread is sufficient.
Also, spurious failures just means we won't recycle that particular object, which is fine.

Result:
Objects no longer risk circulating twice due to a recycle race.

This fixes netty#10986
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 a pull request may close this issue.

3 participants