Eric writes:
Yes, we actually do have code reviews and testing practices. A version of this code was tested successfully prior to this version being merged in, somehow.
Well, that’s ominous. Let’s look at the code.
public static SqsClient create()
{
try {
SqsClient sqsClient = SqsClient.builder() ... .build();
return sqsClient;
} catch (Exception e) {
log.error("SQS - exception creating sqs client", e);
} finally {
// Uncomment this to test the sqs in a test environment
// return SqsClient.builder(). ... .build();
return null;
}
}
Eric found this when he discovered that the application wasn’t sending messages to their queue. According to the logs, there were messages to send, they just weren’t being sent.
Eric made the mistake of looking for log messages around sending messages, when instead he should have been looking at module startup, where the error message above appeared.
This code attempts to create a connection, and if it fails for any reason, it logs an error and returns null. With a delightful “comment this out” for running in the test environment, which, please, god no. Don’t do configuration management by commenting out lines of code. Honestly, that’s the worst thing in this code, to me.
In any case, the calling code “properly” handled nulls by just disabling sending to the queue silently, which made this harder to debug than it needed to be.
[Advertisement]BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
Source: Read MoreÂ