Skip to content

fixed for asyncFinishLock: avoid call finish more than once#3073

Closed
qxo wants to merge 4 commits intoapache:masterfrom
qxo:fix4asyncFinishLock
Closed

fixed for asyncFinishLock: avoid call finish more than once#3073
qxo wants to merge 4 commits intoapache:masterfrom
qxo:fix4asyncFinishLock

Conversation

@qxo
Copy link
Copy Markdown
Contributor

@qxo qxo commented Jul 14, 2019

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • [ *] Bug fix

  • New feature provided

  • Improve performance

  • Related issues


Bug fix

  • Bug description.
    fixed for asyncFinishLock: avoid call finish more than once
  • How to fix?
    call "finish" after asyncFinishLock lock and before unlock

New feature or improvement

  • Describe the details and related test reports.

Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Could you show me how to finish more than once? In my mind, the condition activeSpanStack.isEmpty() && (!isRunningInAsyncMode || asyncSpanCounter.get() == 0) should only be reached once.

@wu-sheng wu-sheng added the TBD To be decided later, need more discussion or input. label Jul 14, 2019
@qxo
Copy link
Copy Markdown
Contributor Author

qxo commented Jul 15, 2019

  1. set breakpoint before call checkFinishConditions and inside checkFinishConditions, then try in debug mode,asyncStop in other thread(pause before checkFinishConditions ), let stopSpan pop make activeSpanStack to empty first,...
  2. It's hard reproduct without codeline debug
  3. I'll do the demo if have the time
  4. In my point of view: The method(checkFinishConditions) code do not change program(object) state,the asyncFinishLock is useless if it's not a problem.
    ===> 理论上一个方法内部没改状态且程序又依赖于其返回值,方法内部加锁是不正确的使用方式:因为多线程在同时请求checkFinishConditions时就会出问题
    如有不对,请指:)

@wu-sheng
Copy link
Copy Markdown
Member

I think I get your point of issue, but if you are talking about #asyncStop and #stopSpan called concurrently, then your solution is still not working, due to the same reason no status change.

I think this problem only could be solved by adding a field #running in TracingContext, default true in the constructor. check running==true before finish condition check, set it to false if check passed. Then add your change, move finish into it.

// rename checkFinishConditions
private void finish () {
     ... lock
     if (running) {
            if (activeSpanStack.isEmpty() && (!isRunningInAsyncMode || asyncSpanCounter.get() == 0)) {
                  running = false;
                  _doFinish(); // rename old finish method
            }
    }
   .... unlock
}

@qxo
Copy link
Copy Markdown
Contributor Author

qxo commented Jul 15, 2019

Yep, my fix still have issue.Please fix it :)

@wu-sheng
Copy link
Copy Markdown
Member

Yep, my fix still have issue.Please fix it :)

By you or me? Do you want me to raise to new PR?

@wu-sheng wu-sheng added agent Language agent related. feature New feature and removed TBD To be decided later, need more discussion or input. labels Jul 15, 2019
@wu-sheng wu-sheng added this to the 6.3.0 milestone Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Language agent related. feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants