7
\$\begingroup\$

I used the idle command to wait for incoming messages in my Gmail mailbox. The protocol I am using is IMAP.

My concern is as follows:

While the below code works, Gmail has a tendency to try to interrupt the connection. Therefore, I found some solutions online including sending a periodic NOOP command and catching the FolderClosedException exception and handling it by reopening the folder and continue listening idly.

This is what I came up with below and would like any feedback:

import java.util.Properties;

import javax.mail.Folder;
import javax.mail.FolderClosedException;
import javax.mail.MessagingException;
import javax.mail.Session;
import javax.mail.Store;
import javax.mail.event.MessageCountEvent;
import javax.mail.event.MessageCountListener;

import com.sun.mail.iap.ProtocolException;
import com.sun.mail.imap.IMAPFolder;
import com.sun.mail.imap.protocol.IMAPProtocol;



public class ImapReconnect {

    private IMAPFolder imapFolder;
    private IMAPFolder processedFolder;
    private IMAPFolder invalidFolder;
    private static final long KEEP_ALIVE_FREQ = 1000;
    private static final String IRIDIUM_MAILBOX_PROCESSED="Processed";
    private static final String IRIDIUM_MAILBOX_INVALID="Invalid";

    private void startService(){
        try {
            setup();
        } catch( MessagingException e) {
            System.out.println("Error configuring imap server:");
            System.out.println(e.toString());
            System.exit(1);
        }

        Thread keepAlive = new Thread(new Runnable(){
            public void run() {
                keepAliveRunner();
            }
        });
        keepAlive.start();

        imapFolder.addMessageCountListener(new MessageCountListener(){

            public void messagesAdded(MessageCountEvent arg0) {
                System.out.println("New message was added.");
            }

            @Override
            public void messagesRemoved(MessageCountEvent arg0) {

            }

        });

        while (!Thread.interrupted()) {
            try {
                imapFolder.idle();
            } catch (FolderClosedException e) {
                System.out.println("The remote server closed the IMAP folder, we're going to try reconnecting.");
                startService();
            } catch (MessagingException e) {
                System.out.println("Now closing imap mailbox, due to unhandlable exception: ");
                System.out.println(e.toString());
                break;
            }      
        }

        if (keepAlive.isAlive()) {
            keepAlive.interrupt();
        }

        try {
            imapFolder.close(false);
            processedFolder.close(false);
            invalidFolder.close(false);
        } catch (MessagingException e) {
            System.out.println("Error closing all the folders:");
            System.out.println(e.toString());
        }
    }

    private void setup() throws MessagingException{
        Properties props = new Properties();
        props.setProperty("mail.store.protocol", "imaps");
        Session session = Session.getInstance(props, null);
        Store store = session.getStore();

        store.connect("imap.googlemail.com", 993,"[email protected]","password");
        imapFolder = (IMAPFolder) store.getFolder("INBOX");

        processedFolder = (IMAPFolder) imapFolder.getFolder(IRIDIUM_MAILBOX_PROCESSED);
        if(!processedFolder.exists())
            processedFolder.create(Folder.HOLDS_MESSAGES);

        invalidFolder = (IMAPFolder) imapFolder.getFolder(IRIDIUM_MAILBOX_INVALID);
        if(!invalidFolder.exists())
            invalidFolder.create(Folder.HOLDS_MESSAGES);

        imapFolder.open(Folder.READ_WRITE);
    }

    public void keepAliveRunner(){
        while (!Thread.interrupted()) {
            try {
                // sleep for 5 minutes
                Thread.sleep(KEEP_ALIVE_FREQ);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            try {
                imapFolder.doCommand(new IMAPFolder.ProtocolCommand() {
                    public Object doCommand(IMAPProtocol p)
                            throws ProtocolException {
                        p.simpleCommand("NOOP", null);
                        return null;
                    }
                });
            } catch (MessagingException e) {
                e.printStackTrace();
            }
        }
    }

    public static void main(String[] args) {
        ImapReconnect reconnect = new ImapReconnect();
        reconnect.startService();
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ What's com.sum.mail.iap? Have you verified that this program works? \$\endgroup\$ Commented Jul 8, 2014 at 5:55
  • \$\begingroup\$ @200_success grepcode.com/file/repo1.maven.org/maven2/javax.mail/mail/1.4.7/… \$\endgroup\$ Commented Jul 8, 2014 at 6:06
  • \$\begingroup\$ Is addMessageCountListener method scan the gmail server all the time ? if yes then is it efficient memory and computation wise ? \$\endgroup\$ Commented Feb 14, 2017 at 16:37

1 Answer 1

3
\$\begingroup\$

Threads:

Threads are hard. It's difficult to do it right and very very easy to mess up. One of the things you shouldn't do are so called Busy Waits.
These busy waits is calls to Thread.sleep() with values other than \$0\$ or \$1\$. You got one with the value \$1000\$

This is a big code-smell and I already got an idea why you did it that way.

Periodically doing stuff

Your design is somewhat off. Usually the checking of mailbox is accomplished a little differently than keeping the IMAP connection open.

Instead you once synchronize your folders and then have a scheduler redo that in the background again and again.
The standard way to do this seems to be (no experience on my side) to authenticate every request to the SMTP-Server.

You can accomplish this by using ScheduledExecutorService interface. There is a method scheduleAtFixedRate() provided. I suggest you use that instead of going for Task. Your code would then change from:

    //... some code
    Thread keepAlive = new Thread(new Runnable(){
        public void run() {
            keepAliveRunner();
        }
    });
    keepAlive.start();
    //... other code ;)
}

public void keepAliveRunner(){
    while (!Thread.interrupted()) {
        try {
            // sleep for 5 minutes
            Thread.sleep(KEEP_ALIVE_FREQ);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        try {
            imapFolder.doCommand(new IMAPFolder.ProtocolCommand() {
                public Object doCommand(IMAPProtocol p)
                        throws ProtocolException {
                    p.simpleCommand("NOOP", null);
                    return null;
                }
            });
        } catch (MessagingException e) {
            e.printStackTrace();
        }
    }
}

to:

    //... some code
    ScheduledExecutorService keepAlive = Executors.newScheduledThreadPool(1);
    Runnable toKeepAlive = new Runnable() {
        public void run() {
            keepAliveRunner();
        }
    };
    keepAlive.scheduleAtFixedRate(toKeepAlive,
          KEEP_ALIVE_FREQ, 
          KEEP_ALIVE_FREQ, 
          TimeUnit.MILLISECONDS); 
    /* you could use seconds / minutes instead, but then
    You'd need to change KEEP_ALIVE_FREQ ;) */
    //... other code ;)
}

public void keepAliveRunner() {
   try {
       imapFolder.doCommand(new IMAPFolder.ProtocolCommand() {
           public Object doCommand(IMAPProtocol p)
                   throws ProtocolException {
               p.simpleCommand("NOOP", null);
               return null;
           }
       });
    } catch (MessagingException e) {
      e.printStackTrace();
    }
}

This should accimplish the same, but doesn't block a whole thread with a busy wait (which isn't nice for your CPU). Also this largely reduces the complexity of your keepAliveRunner() method, which I would rename to runKeepAliveRequest() now.

This does not incorporate the changes I suggested earlier. I have no idea how the rest of your code works, so I just leave it like that and hope you can use it ;)

\$\endgroup\$
4
  • \$\begingroup\$ Quick question. With this recommended change, what should I do with this: if (keepAlive.isAlive()) { keepAlive.interrupt(); } \$\endgroup\$ Commented Jul 8, 2014 at 16:18
  • \$\begingroup\$ Should i replace it with keepAlive.shutdown()? \$\endgroup\$ Commented Jul 8, 2014 at 16:20
  • \$\begingroup\$ One final thought if I a may. One of my main concerns was if FolderClosedException is thrown. Is doing a recursive call to startService() to rebound from the exception a workable thing here? \$\endgroup\$ Commented Jul 8, 2014 at 16:39
  • \$\begingroup\$ @JohnMerlino concerning keepAlive.shutdown(), Yes, definitely. Concerning FolderClosedException: no clue, I don't know the specifics of IMAP and how you handle that. Also that might depend on what exactly you want to accomplish (sorry for the long pipeline) \$\endgroup\$
    – Vogel612
    Commented Jul 9, 2014 at 8:56

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.