Potential NPE in Spring module when handling exceptions

[altifuse]

When intercepted code throws exceptions, these are added to the current segment:

aws-xray-sdk-java/aws-xray-recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/AbstractXRayInterceptor.java

Lines 54 to 55 in f893f6c

	} catch (Exception e) {
	AWSXRay.getCurrentSegment().addException(e);

However, when the segment does not exist, I'm experiencing NPEs (using v2.2.1). This seems similar to #41, which referenced the same method, but this is a different case.

[chanchiem]

Hey,

Thanks for reporting the issue you are having. The expected behavior for this would be that the call to AWSXRay.getCurrentSegment() would throw a SegmentNotFoundException in the call to getCurrentSegment() if the context missing strategy was to throw runtime exceptions.

It looks like you've identified an edge case where if for some reason, the code path for getCurrentSegment() is allowed to continue executing (and thereby return a null object), then the call to addException() would still occur for a null object, thus throwing a NPE.

I think a fix would be to have an explicit check for this case to ensure that the object returned isn't null in case if the customer has a configuration to not throw exceptions (such as LogErrorContextMissingStrategy).

So instead of

aws-xray-sdk-java/aws-xray-recorder-sdk-spring/src/main/java/com/amazonaws/xray/spring/aop/AbstractXRayInterceptor.java

Lines 54 to 57 in f893f6c

	} catch (Exception e) {
	AWSXRay.getCurrentSegment().addException(e);
	throw e;
	} finally {

It should be

        } catch (Exception e) {
            Segment currentSegment = AWSXRay.getCurrentSegment();
            if (currentSegment != null) {
                currentSegment.addException(e);
            }
            throw e;
        } finally {

Some things I wanted to ask to verify some of the assumptions above:

• Are you experiencing any other exceptions like the ContextMissingException?
• What context missing strategy are you using? LogErrorContextMissingStrategy?

Thanks!

[altifuse]

Thanks for the response, @chanchiem.

I agree with your proposed fix. From what I've gathered that would be enough.

Regarding your questions:

Are you experiencing any other exceptions like the ContextMissingException?
What context missing strategy are you using? LogErrorContextMissingStrategy?

Yes, that's the strategy we are using. There are logs mentioning the suppression of SubsegmentNotFoundException and SegmentNotFoundException.

[altifuse]

Are there any updates on this?

[chanchiem]

Hey, we have this in our priority and will come out with a PR. Please stay tuned.