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

Fix #1549, spring-plugins bug #1554

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ylywyn

ylywyn commented Aug 17, 2018

ContextManager.stopSpan(); may not execute

Please answer these questions before submitting pull request

  • Why submit this pull request?
    Bug fix

  • Related issues


Bug fix

  • Bug description.
  • How to fix?
Fix #1549, spring-plugins bug
ContextManager.stopSpan(); may not execute

@wu-sheng wu-sheng requested a review from ascrutae Aug 17, 2018

@wu-sheng wu-sheng added this to the 5.0.0-RC milestone Aug 17, 2018

@ascrutae

I think that this is not the root cause about #1549, I suspect that the problem is caused by that the plugin only stores the Request and does not store Response。because of the plugin had check if Request is empty when creating span in beforeMethod and the plugin check the Response is empty when finish the span in afterMethod.

@wu-sheng

This comment has been minimized.

Show comment
Hide comment
@wu-sheng

wu-sheng Aug 17, 2018

Member

I think that this is not the root cause about #1549, I suspect that the problem is caused by that the plugin only stores the Request and does not store Response。because of the plugin had check if Request is empty when creating span in beforeMethod and the plugin check the Response is empty when finish the span in afterMethod.

Can you provide more detail or another fix for this? If plugin checked, where does the span come from? Any other plugin leak the span?

Member

wu-sheng commented Aug 17, 2018

I think that this is not the root cause about #1549, I suspect that the problem is caused by that the plugin only stores the Request and does not store Response。because of the plugin had check if Request is empty when creating span in beforeMethod and the plugin check the Response is empty when finish the span in afterMethod.

Can you provide more detail or another fix for this? If plugin checked, where does the span come from? Any other plugin leak the span?

@wu-sheng

This comment has been minimized.

Show comment
Hide comment
@wu-sheng

wu-sheng Aug 17, 2018

Member

@ylywyn Look like @ascrutae has a different thought, maybe this is not the right way to fix. But the bug is real.

@ascrutae Please provide the details and things which need @ylywyn to recheck for you.

Member

wu-sheng commented Aug 17, 2018

@ylywyn Look like @ascrutae has a different thought, maybe this is not the right way to fix. But the bug is real.

@ascrutae Please provide the details and things which need @ylywyn to recheck for you.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 17, 2018

Coverage Status

Coverage increased (+0.004%) to 24.686% when pulling d1c22b5 on ylywyn:master into dd942d1 on apache:master.

coveralls commented Aug 17, 2018

Coverage Status

Coverage increased (+0.004%) to 24.686% when pulling d1c22b5 on ylywyn:master into dd942d1 on apache:master.

@ascrutae

This comment has been minimized.

Show comment
Hide comment
@ascrutae

ascrutae Aug 17, 2018

Member

@ylywyn Because the Response and Request are not stored in RuntimeContext at the same time, please see InvokeForRequestInterceptor and GetBeanInterceptor.java, so I think that The Response interceptor is not in effect in some version. Could you give the demo to reproduce this issue. or you can check if the Response is stored in the RuntimeContext

Member

ascrutae commented Aug 17, 2018

@ylywyn Because the Response and Request are not stored in RuntimeContext at the same time, please see InvokeForRequestInterceptor and GetBeanInterceptor.java, so I think that The Response interceptor is not in effect in some version. Could you give the demo to reproduce this issue. or you can check if the Response is stored in the RuntimeContext

@wu-sheng wu-sheng removed this from the 5.0.0-RC milestone Aug 18, 2018

@wu-sheng

This comment has been minimized.

Show comment
Hide comment
@wu-sheng

wu-sheng Aug 18, 2018

Member

@ylywyn Need you to work with @ascrutae . His thoughts are reasonable.

Member

wu-sheng commented Aug 18, 2018

@ylywyn Need you to work with @ascrutae . His thoughts are reasonable.

@ylywyn

This comment has been minimized.

Show comment
Hide comment
@ylywyn

ylywyn Aug 20, 2018

yes, this is not the root cause about #1549. context has been removed before AbstractMethodInterceptor::afterMethod.

callstack like this:
6: InvokeForRequestInterceptor::afterMethod

5: AbstractMethodInterceptor::afterMethod context[] is null . ContextManager.stopSpan() can't be call
4: AbstractMethodInterceptor::beforeMethod

3: InvokeForRequestInterceptor::afterMethod context[SW_REQUEST] -> context[]
2: InvokeForRequestInterceptor::beforeMethod context[SW_REQUEST, SW_RESPONSE] -> context[SW_REQUEST, SW_RESPONSE]

1: InvokeForRequestInterceptor::beforeMethod context[SW_REQUEST] -> context[SW_REQUEST, SW_RESPONSE]

ylywyn commented Aug 20, 2018

yes, this is not the root cause about #1549. context has been removed before AbstractMethodInterceptor::afterMethod.

callstack like this:
6: InvokeForRequestInterceptor::afterMethod

5: AbstractMethodInterceptor::afterMethod context[] is null . ContextManager.stopSpan() can't be call
4: AbstractMethodInterceptor::beforeMethod

3: InvokeForRequestInterceptor::afterMethod context[SW_REQUEST] -> context[]
2: InvokeForRequestInterceptor::beforeMethod context[SW_REQUEST, SW_RESPONSE] -> context[SW_REQUEST, SW_RESPONSE]

1: InvokeForRequestInterceptor::beforeMethod context[SW_REQUEST] -> context[SW_REQUEST, SW_RESPONSE]

@ylywyn

This comment has been minimized.

Show comment
Hide comment
@ylywyn

ylywyn Aug 20, 2018

RUNTIME_CONTEXT management (put and remove) seems to have some problems.

ylywyn commented Aug 20, 2018

RUNTIME_CONTEXT management (put and remove) seems to have some problems.

@ascrutae

This comment has been minimized.

Show comment
Hide comment
@ascrutae

ascrutae Aug 20, 2018

Member

I know it, @ylywyn just remove the clear RuntimeContext code in InvokeForRequestInterceptor#afterMethod . Don't worry about leak memory cause by RuntimeContext, it will clear in AbstractMethodInterceptor#afterMethod.

Member

ascrutae commented Aug 20, 2018

I know it, @ylywyn just remove the clear RuntimeContext code in InvokeForRequestInterceptor#afterMethod . Don't worry about leak memory cause by RuntimeContext, it will clear in AbstractMethodInterceptor#afterMethod.

@ylywyn

This comment has been minimized.

Show comment
Hide comment
@ylywyn

ylywyn Aug 20, 2018

Moving ContextManager.stopSpan() to finally{} can solve my problem.
I am not a Java (Spring) developer, so I am not sure that this method AbstractMethodInterceptor::afterMethod will definitely be called under any conditions.

ylywyn commented Aug 20, 2018

Moving ContextManager.stopSpan() to finally{} can solve my problem.
I am not a Java (Spring) developer, so I am not sure that this method AbstractMethodInterceptor::afterMethod will definitely be called under any conditions.

@wu-sheng wu-sheng added this to the 5.0.0-RC milestone Aug 20, 2018

@ascrutae

This comment has been minimized.

Show comment
Hide comment
@ascrutae

ascrutae Aug 20, 2018

Member

The 5.0.0-RC will release in this weekend. I hope that this issue will be fixed in this issue , And could you update this PR? @ylywyn

Member

ascrutae commented Aug 20, 2018

The 5.0.0-RC will release in this weekend. I hope that this issue will be fixed in this issue , And could you update this PR? @ylywyn

ascrutae and others added some commits Aug 20, 2018

@wu-sheng wu-sheng removed this from the 5.0.0-RC milestone Aug 21, 2018

@wu-sheng

This comment has been minimized.

Show comment
Hide comment
@wu-sheng

wu-sheng Aug 21, 2018

Member

The 5.0.0-RC will release in this weekend. I hope that this issue will be fixed in this issue , And could you update this PR? @ylywyn

@ascrutae Consider sending a separated PR to fix this? I will keep this PR open in next 24 hours, in case @ylywyn still has interested in the update.

Member

wu-sheng commented Aug 21, 2018

The 5.0.0-RC will release in this weekend. I hope that this issue will be fixed in this issue , And could you update this PR? @ylywyn

@ascrutae Consider sending a separated PR to fix this? I will keep this PR open in next 24 hours, in case @ylywyn still has interested in the update.

@ascrutae ascrutae referenced this pull request Aug 21, 2018

Merged

Fix #1549 #1568

1 of 3 tasks complete
@wu-sheng

This comment has been minimized.

Show comment
Hide comment
@wu-sheng

wu-sheng Aug 21, 2018

Member

#1568 provided a better solution to fix this. @ylywyn still thanks for helping.

Member

wu-sheng commented Aug 21, 2018

#1568 provided a better solution to fix this. @ylywyn still thanks for helping.

@wu-sheng wu-sheng closed this Aug 21, 2018

@wu-sheng wu-sheng added this to the 5.0.0-RC milestone Aug 21, 2018

@wu-sheng wu-sheng added the wontfix label Aug 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment