1
\$\begingroup\$

I have a requirement to send emails when certain errors are logged(using log4j 1.2.17). But the SMTP appender sends emails in a synchronized manner. I can not use the AsyncAppender either. So I have written my own custom appender and an email sender which utilizes the javax.mail library. The custom appender creates a fixed thread pool of 5 threads using the ExecutorService, and in the custom Appenders append() method I create an object of the email sender class that I have written and submit that to the thread pool.

The Code for the custom appender is as following(some of the non-important segments have been removed eg: some parameters when creating an instance of emailSender)

public class emailAppender extends AppenderSkeleton {


    private static ExecutorService threadPool = null;

    static {
        threadPool = Executors.newFixedThreadPool(5);
    }
    @Override
    protected void append(LoggingEvent event) {
        if(this.closed != true){
            emailSender emailNotification = new emailSender(
                   event.getMessage().toString(),getReceiver());
            threadPool.submit(emailNotification);
        }
    }

    public void close() {
        this.closed=true;
    }

    public boolean requiresLayout() {
        return false;
    }
}

The code for the email sender is as follows

public class emailSender implements Runnable  {

    public emailSender( password, String message, String receiver){
        this.message=message;
        this.receiver=receiver;
    }

    public void run() {
        Properties props = new Properties();
        props.put("mail.smtp.starttls.enable", "true");
        props.put("mail.smtp.port", getSmtpPort());

        Session session = Session.getDefaultInstance(props,
                new javax.mail.Authenticator() {
                    protected PasswordAuthentication getPasswordAuthentication() {
                        return new PasswordAuthentication(getSmtpUsername(),getSmtpPassword());
                    }
                });

        try {
            MimeMessage message = new MimeMessage(session);
            message.setFrom(new InternetAddress(getSmtpUsername()));
            message.addRecipient(Message.RecipientType.TO,new InternetAddress(getReceiver()));
            message.setSubject(getSubject());
            message.setText(getMessage());
            Transport.send(message);
            System.out.println("Notification sent successfully to " + getReceiver() + " for Error : " + getMessage());
        } catch (MessagingException e) {
            e.printStackTrace();
        }
    }

The size of the thread pool was determined considering the maximum number of errors that get logged at one time. And also the downside of the solution being asynchronous is that the logs might not actually get emailed which is acceptable for us.

Currently, this code works fine for me. I would want to know if this code is acceptable for production without considering the fact that the logs might not get emailed due to asynchronous behavior.

\$\endgroup\$
1
  • \$\begingroup\$ Welcome to CodeReview@SE! Pretty good presentation of how context limited choice in design&implementation. \$\endgroup\$
    – greybeard
    Commented Jan 12, 2020 at 12:54

1 Answer 1

0
\$\begingroup\$

The UpperCamelCase format is used to name types. You should rename your classes (and constructors) to EmailAppender and EmailSender.

The ExecutorService has a shutdown method that you can use to reject new tasks and terminate the current ones. You may shutdown your pool form the close method. And there is a nice code sample in the official documentation : https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ExecutorService.html

I guess you should also avoid to use System.out.println and e.printStackTrace(). They will both print unexpected messages on the console. Imagine that your code will be shipped in a library, once a dev will use your library he will suddenly got things printed on his console.

Aside of that I don't see anything wrong.

On a more personal point of view, I would have tried to extract the "configuration" (Creation of Properties and creation of the instance) to another class so that you make a clear distinction between the conversion of the LoggingEvent to an email and the sending of an email message.

\$\endgroup\$
2
  • \$\begingroup\$ should I be using pool.shutdown() or pool.shutdownNow() if the logging should happen until the server is shut down? I do not understand if there is a situation that I would want to explicitly stop receiving new tasks and terminating the current tasks. \$\endgroup\$ Commented Jan 16, 2020 at 11:44
  • \$\begingroup\$ Well, it is up to you. But teh documentation provide a great exampel of gracefull shutdown. \$\endgroup\$
    – gervais.b
    Commented Jan 20, 2020 at 10:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.