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

fix npe in guava eventbus plugin #559

Merged
merged 2 commits into from
Jun 21, 2023
Merged

fix npe in guava eventbus plugin #559

merged 2 commits into from
Jun 21, 2023

Conversation

xzyJavaX
Copy link
Contributor

@xzyJavaX xzyJavaX commented Jun 20, 2023

@wu-sheng
Copy link
Member

I think we need to explain why the context could be either active or not?

@wu-sheng wu-sheng added this to the 8.17.0 milestone Jun 20, 2023
@wu-sheng wu-sheng added bug Something isn't working plugin labels Jun 20, 2023
@wu-sheng
Copy link
Member

@xzyJavaX Any update?

@xzyJavaX
Copy link
Contributor Author

When start a new thread to call asyncEventBus.post(testEvent), the context will be inactive, this situation has no context.

@wu-sheng
Copy link
Member

So, in this case, the posting message is not traced? Isn't this a bug rather than fix NPE?

@lujiajing1126
Copy link
Contributor

lujiajing1126 commented Jun 21, 2023

So, in this case, the posting message is not traced? Isn't this a bug rather than fix NPE?

I suppose It is traced but orphan. When calling ContextManager.capture to capture the state of TracingContext in the current thread, it throws NPE since the TracingContext does not exist in a new thread.

I think it is users' responsibility to propagate tracing context in this case, e.g. use @TraceCrossThread.

This PR can only avoid NPE to be thrown.

@wu-sheng
Copy link
Member

Make sense. Please fix the UT failing.

@wu-sheng wu-sheng merged commit b72e984 into apache:main Jun 21, 2023
yangyulely pushed a commit to yangyulely/skywalking-java that referenced this pull request May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants