Skip to content

Conversation

@felixbarny
Copy link
Member

@felixbarny felixbarny commented Sep 17, 2018

closes #178

@felixbarny
Copy link
Member Author

No tests because I can't reproduce it locally, and it only happened with the intake v1 reporter, which is about to be deprecated.

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

I thought of creating a small and simple abstraction layer in front of the ObjectPool that will guard us from getting that again, for example add:

    abstract class Poolable implements Recyclable {
        static final long  NON_USED = -1;
        
        volatile long usingThread = NON_USED;
        
        Poolable setUsed() {
            usingThread = Thread.currentThread().getId();
            return this;
        }
        
        boolean setUnused() {
            boolean stateChanged = false;
            if (Thread.currentThread().getId() == usingThread) {
                usingThread = NON_USED;
                stateChanged = true;
            }
            return stateChanged;
        }
    }

and make all poolable objects extend it and only allow getting and returning objects through one place. I think this is currently the state with ElasticApmTracer for example. And then do something like:

    class ElasticApmTracer {
        ....
        public Transaction startTransaction(@Nullable String traceContextHeader, Sampler sampler, long nanoTime){
            ....
            transaction = transactionPool.createInstance().setUsed().start(traceContextHeader, nanoTime, sampler);
            ....
            return transaction;
        }

        public void recycle(Transaction transaction) {
            ....
            if (transaction.setUnused()) {
                transactionPool.recycle(transaction);
            }
            ....
        }
    }

I think that assuming the same thread won't try to return an object twice while trying to get an object in between, this is thread safe (since one thread cannot return an object it didn't take). I would also add to such abstraction layer an atomic counter to count all object currently in use so we can track leaks. What do you say?

@felixbarny
Copy link
Member Author

Good ideas! Let's do that but in another PR. I would also make it transparent for the tracer and let the object pool do the work of calling setUsed() and setUnused.

@felixbarny felixbarny merged commit 99b4a3c into elastic:master Sep 18, 2018
@felixbarny felixbarny deleted the fix-recycle-twice branch September 18, 2018 07:47
felixbarny added a commit that referenced this pull request Sep 18, 2018
@SylvainJuge SylvainJuge added bug Bugs and removed type: bug labels Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect transaction duration

4 participants