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

PAYARA-3928 Fix NPE when notification via CDI eventbus comes in #4043

Merged
merged 1 commit into from Jun 14, 2019

Conversation

@arjantijms
Copy link
Contributor

commented Jun 13, 2019

Signed-off-by: arjantijms arjan.tijms@gmail.com

PAYARA-3928 Fix NPE when notification via CDI eventbus comes in
Signed-off-by: arjantijms <arjan.tijms@gmail.com>

@arjantijms arjantijms added this to the 5.193 milestone Jun 13, 2019

@arjantijms arjantijms requested review from pdudits and Pandrex247 Jun 13, 2019

@arjantijms arjantijms self-assigned this Jun 13, 2019

@arjantijms

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Jenkins test please

@@ -236,7 +236,7 @@ else if(backupClassLoader != null) {
transactionManager.clearThreadTx();
}

if (requestTracing != null && requestTracing.isRequestTracingEnabled()) {
if (requestTracing != null && requestTracing.isRequestTracingEnabled() && handle.getSpanContextMap() != null) {

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Jun 13, 2019

Contributor

Shouldn't this NULL check be in the startConcurrentContextSpan method.

This comment has been minimized.

Copy link
@arjantijms

arjantijms Jun 13, 2019

Author Contributor

Would that make a lot of difference?

The check would have to be here then:

private void startConcurrentContextSpan(ComponentInvocation invocation, InvocationContext handle) {
    if (handle.getSpanContextMap() == null) {
        return;
    }
    Tracer tracer = openTracing.getTracer(openTracing.getApplicationName(
            Globals.getDefaultBaseServiceLocator().getService(InvocationManager.class)));

    SpanContext spanContext = tracer.extract(Format.Builtin.TEXT_MAP, new MapToTextMap(handle.getSpanContextMap()));

To my mind the check is at a similar level to request tracing being enabled (globally). If there's no span context map, then request tracing is not active for the current context/thread. In both cases we shouldn't even attempt to start a concurrent context span.

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Jun 13, 2019

Contributor

I just thought that the reason for the check would be clearer if it is close to the usage. And you wouldn't need to check it again if the method would be called anywhere else.

This comment has been minimized.

Copy link
@arjantijms

arjantijms Jun 14, 2019

Author Contributor

It's not a wrong point indeed, although what the check is actually asking for is

"isTracingLocallyActive()"

From a semantic point of view it would be:

if (isTracingGloballyEnabled() && isTracingLocallyActive()) {
   startConcurrentContextSpan();
}

vs

startConcurrentContextSpan();

And then inside startConcurrentContextSpan:

private void startConcurrentContextSpan(ComponentInvocation invocation, InvocationContext handle) {
    if (!isTracingGloballyEnabled() || !isTracingLocallyActive()) {
        return;
    }
    ...
}

In other words, always (or rather, often) call methods unconditionally, then check inside the methods right away if they should be called.

Might be best to discuss this as a base practice for the entire code convention. I'll merge this as is now, but certainly take the consideration on board.

thx!

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Jun 14, 2019

Member

I'm not 100% convinced on this change - I also think it should be moved inside the startConcurrentContextSpan method.

We very recently had a similar change being suggested by Jan (via Sonar) about the ComponentInvocation being null, and I think I want to echo the same response: we still want to start a span, just exclude the stuff we can't get.
Reason being is that this is the entrypoint for essentially all async spans - the ramifications of not starting a span here will echo outwards.

So in this case, I'd suggest just editing the method so that it works a bit more like the JaxrsClientRequestTracingFilter class (see snippit below):


SpanContext parentSpanContext = (SpanContext) requestContext.getProperty(
                        PropagationHeaders.OPENTRACING_PROPAGATED_SPANCONTEXT);
                if (parentSpanContext != null) {
                    spanBuilder.asChildOf(parentSpanContext);
}

So try and get the context from the InvocationContext, and create a span as a child of it if there is one, otherwise just create a span with no parent.

@jbee
jbee approved these changes Jun 14, 2019
@pdudits
Copy link
Contributor

left a comment

Approving, but take @svendiedrichsen proposal into consideration yet.

It's a general style in entire codebase to have caller do lots of sanity checks, but I think this leaks implementation detail of the method.

@arjantijms arjantijms merged commit 69c4de8 into payara:master Jun 14, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details

@arjantijms arjantijms deleted the arjantijms:PAYARA-3928-CDI-event-npe branch Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.