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

FixedChannelPool does not count acquired channels precisely #3988

Closed
alexpark7712 opened this issue Jul 15, 2015 · 9 comments
Closed

FixedChannelPool does not count acquired channels precisely #3988

alexpark7712 opened this issue Jul 15, 2015 · 9 comments
Assignees
Labels
Milestone

Comments

@alexpark7712
Copy link
Contributor

Netty version: 4.0.10.Beta5

Context:
When queued AcquireTask fails, decrementAndRunTaskQueue() method could throw assertion error.

Steps to reproduct:

  1. Call FixedChannelPool.acquire enough to make some AcquireTasks be queued.
  2. Close server channel abrubtly to make AcquireTask fail.
  3. AssertionError can be produced.
assert acquiredChannelCount >= 0;

Opinion:

  • Set a flag acquired in AcquireListener and set it true when it is dequed from pendingAcquireQueue.
  • When acquire future is failed, if acquired flag is true call decrementAndRunTaskQueue() else call runTaskQueue()
    private class AcquireListener implements FutureListener<Channel> {
        private final Promise<Channel> originalPromise;
        protected boolean acquired;

        AcquireListener(Promise<Channel> originalPromise) {
            this.originalPromise = originalPromise;
            acquired = true;
        }

        @Override
        public void operationComplete(Future<Channel> future) throws Exception {
            assert executor.inEventLoop();

            if (future.isSuccess()) {
                originalPromise.setSuccess(future.getNow());
            } else {
                // Something went wrong try to run pending acquire tasks.
                if(acquired) {
                    decrementAndRunTaskQueue();
                } else {
                    runTaskQueue();
                }
                originalPromise.setFailure(future.cause());
            }
        }
    }

    private final class AcquireTask extends AcquireListener {
        final Promise<Channel> promise;
        final long expireNanoTime = System.nanoTime() + acquireTimeoutNanos;
        ScheduledFuture<?> timeoutFuture;

        public AcquireTask(Promise<Channel> promise) {
            super(promise);
            // We need to create a new promise as we need to ensure the AcquireListener runs in the correct
            // EventLoop.
            this.promise = executor.<Channel>newPromise().addListener(this);
            acquired = false;
        }
    }

    private void runTaskQueue() {
        while (acquiredChannelCount < maxConnections) {
            AcquireTask task = pendingAcquireQueue.poll();
            if (task == null) {
                break;
            }

            // Cancel the timeout if one was scheduled
            ScheduledFuture<?> timeoutFuture = task.timeoutFuture;
            if (timeoutFuture != null) {
                timeoutFuture.cancel(false);
            }

            task.acquired = true;

            --pendingAcquireCount;
            ++acquiredChannelCount;

            super.acquire(task.promise);
        }

        // We should never have a negative value.
        assert pendingAcquireCount >= 0;
        assert acquiredChannelCount >= 0;
    }
@alexpark7712 alexpark7712 changed the title FixedChannelPool does not count acquired channels preceisely FixedChannelPool does not count acquired channels precisely Jul 15, 2015
@Scottmitch Scottmitch added this to the 4.0.30.Final milestone Jul 15, 2015
@Scottmitch
Copy link
Member

@fratboy - Thanks for reporting!

@normanmaurer - Assigned to you for now.

@normanmaurer
Copy link
Member

@fratboy thanks... fix in the works.

@alexpark7712
Copy link
Contributor Author

@normanmaurer My pleasure.

normanmaurer added a commit that referenced this issue Jul 21, 2015
Motivation:

We missed to correctly count acquired channels in FixedChannelPool which could produce an assert error.

Modifications:

Only try to decrement acquired count if the channel was really acuired.

Result:

No more assert error possible.
@normanmaurer
Copy link
Member

@fratboy could you verify #4016 ?

@alexpark7712
Copy link
Contributor Author

@normanmaurer It's exactly same with my patch in my project. It works perfect.

normanmaurer added a commit that referenced this issue Jul 21, 2015
Motivation:

We missed to correctly count acquired channels in FixedChannelPool which could produce an assert error.

Modifications:

Only try to decrement acquired count if the channel was really acuired.

Result:

No more assert error possible.
normanmaurer added a commit that referenced this issue Jul 21, 2015
Motivation:

We missed to correctly count acquired channels in FixedChannelPool which could produce an assert error.

Modifications:

Only try to decrement acquired count if the channel was really acuired.

Result:

No more assert error possible.
normanmaurer added a commit that referenced this issue Jul 21, 2015
Motivation:

We missed to correctly count acquired channels in FixedChannelPool which could produce an assert error.

Modifications:

Only try to decrement acquired count if the channel was really acuired.

Result:

No more assert error possible.
@Scottmitch
Copy link
Member

@fratboy - Thanks for the report and verification!

@wyzssw
Copy link

wyzssw commented Jan 1, 2019

Why pendingAcquireCount use int not AtomicInteger ? @normanmaurer @fratboy

@normanmaurer
Copy link
Member

@wyzssw because everything is done from the same Thread (EventLoop)

@wyzssw
Copy link

wyzssw commented Jan 1, 2019

@normanmaurer I get it, thanks

pulllock pushed a commit to pulllock/netty that referenced this issue Oct 19, 2023
Motivation:

We missed to correctly count acquired channels in FixedChannelPool which could produce an assert error.

Modifications:

Only try to decrement acquired count if the channel was really acuired.

Result:

No more assert error possible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants