Rewrite IMAP IDLE #2671

rhari991 · 2017-08-09T12:06:37Z

This PR builds upon the changes made to ImapFolderPusher in PR #2607 by

Removing all the code that processes the various IDLE responses
Making it not extend ImapFolder anymore
Not sending DONE and IDLE commands whenever a useful response is received through IDLE

philipwhiuk · 2017-08-13T11:00:05Z

I'm happy to review this, but there's not much point until the tests pass..

Valodim · 2017-08-13T21:07:17Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolderPusher.java

    private static final int IDLE_READ_TIMEOUT_INCREMENT = 5 * 60 * 1000;
    private static final int IDLE_FAILURE_COUNT_LIMIT = 10;
    private static final int MAX_DELAY_TIME = 5 * 60 * 1000; // 5 minutes
    private static final int NORMAL_DELAY_TIME = 5000;

-
+    private final ImapFolder folder;


Just a thought: there aren't a lot of references to this folder object, and those I saw either worked on the ImapConnection or ImapStore, and especially in the case of ImapConnection we partly manage that object ourselves here which leads to weird coupling between the classes. Perhaps we can easily get rid of this dependency here?

I'm not sure how to do that, since the folder / the folder name needs to saved in between calls to the constructor and the start() / stop() methods. Since each instance of ImapFolderPusher manages a single folder, I think it is appropriate to have it as an instance variable.

rhari991 · 2017-08-18T03:16:51Z

k9mail-library/src/test/java/com/fsck/k9/mail/store/imap/ImapFolderPusherTest.java

+
+        folderPusher.start();
+
+        verify(pushReceiver).syncFolder(FOLDER_NAME);


@Valodim When I debug this test, this invocation does occur, but this verification still fails. Is there something I am missing here ?

Valodim

Good start, I like that ImapFolderPusher lost quite a bit of weight. I don't quite like how the object uses both ImapFolder and its underlying ImapConnection at the moment. Ideally it should access the folder, or the connection object, not both. It's possible that that's too complicated, please look into it.

Valodim · 2017-08-21T16:14:45Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolderPusher.java

-                Timber.v("Got async response: %s", response);
-            }
-
+        public void handleAsyncUntaggedResponse(ImapResponse response) throws IOException, MessagingException {


while you're at it, please change this into an anonymous inner class rather than having PushRunnable implement the interface. that way the executeSimpleCommand(Commands.IDLE, this) becomes executesimpleCommand(Commands.IDLE, untaggedResponseHandler), which makes it easier to trace where the handling happens in the IDE.

e.g.

private UntaggedHandler untaggedResponseHandler = new UntaggedHandler() { ... }

(also, is this really "async"? it's a callback that is called in the thread that runs executeSimpleCommand, isn't it?)

I think it is async in the sense that executeSimpleCommand(Commands.IDLE, untaggedhandler) is a long blocking call, and while it is blocking, handleAsyncUntaggedResponse is called to process a response.

Valodim · 2017-08-21T16:15:46Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolderPusher.java

+                            synchronized (storedUntaggedResponses) {
+                                storedUntaggedResponses.add(response);
+                                if (!folder.getConnection().areMoreResponsesAvailable()) {
+                                    processStoredUntaggedResponses();


this entire block is conditional (response.size() and isUntaggedResponseSupported above), but it should probably always trigger if no more responses are available

this is changed, but not fixed I think

@Valodim Sorry, I thought you were talking about the indentation. I have moved the processing command to the end of the method.

Valodim · 2017-08-21T16:54:51Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolderPusher.java

@@ -327,10 +298,10 @@ private void returnFromIdle() {
        }

        private boolean openConnectionIfNecessary() throws MessagingException {
-            ImapConnection oldConnection = connection;
-            internalOpen(OPEN_MODE_RO, INVALID_UID_VALIDITY, INVALID_HIGHEST_MOD_SEQ);
+            ImapConnection oldConnection = folder.getConnection();


doing this from a getter is a hack, see comment above on the ImapConnection responsibility

Removed the getter.

Valodim · 2017-08-21T16:56:01Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolderPusher.java

                        if (K9MailLib.isDebug()) {
                            Timber.i("About to IDLE for %s", getLogId());
                        }

                        prepareForIdle();

-                        ImapConnection conn = connection;
+                        ImapConnection conn = folder.getConnection();
                        setReadTimeoutForIdle(conn);


the responsibility of ImapConnection is weirdly distributed here: the timeout is set directly on the connection from ImapFolder, but the executeSimpleCommand then is run by the folder itself. is the connection supposed to be managed by the ImapFolder, or not?

If it is, we should try to do as little on this object as possible, ideally get rid of the getter. But manipulating the ImapConnection object that is otherwise managed by ImapFolder feels wrong.

There are some places where it is appropriate to manage the connection directly, like setReadTimeout and not through the folder. I decided to create a new class IdleConnectionManager that wraps an ImapConnection object and exposes only the necessary method to ImapFolderPusher. I also moved all the code from the IdleStopper class into it.

Valodim · 2017-08-21T16:59:57Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolderPusher.java

+                        untaggedResponses.size(), getLogId());
+            }
+
+            for (int i = 0; i < untaggedResponses.size();i++) {


(code style)

lucafavatella · 2017-10-07T15:15:41Z

I have the impression this PR was rendered meaningless by an erroneous force-push potentially related to PR 2701, because:

I see title with "IDLE", but no IDLE-related changes;
I see review comments on file ImapFolderPusher.java now absent.

I see this PR at commit 1dcbaa7.

I suggest ensuring an issue is open by distilling PR title and PR description, and then closing this PR.

philipwhiuk · 2017-10-07T22:04:20Z

@lucafavatella This is part of a GSoC project finalisation step. There's already issues surrounding it. It'll be closed when it's merged / @rhari991 finishes the hard work they have done surrounding IDLE and CONDSTORE.

rhari991 requested review from jberkel and Valodim and removed request for jberkel August 9, 2017 19:02

Valodim reviewed Aug 13, 2017

View reviewed changes

rhari991 force-pushed the condstore-support branch from 68a688b to cf1bc5d Compare August 14, 2017 14:40

rhari991 force-pushed the rewrite-imap-idle branch 4 times, most recently from 0caf0c0 to ecc6a66 Compare August 18, 2017 02:40

rhari991 commented Aug 18, 2017

View reviewed changes

rhari991 force-pushed the rewrite-imap-idle branch 6 times, most recently from be32ac2 to 5660ab0 Compare August 20, 2017 17:42

Valodim suggested changes Aug 21, 2017

View reviewed changes

rhari991 force-pushed the rewrite-imap-idle branch from c296d8a to 033f702 Compare August 22, 2017 00:11

rhari991 force-pushed the condstore-support branch from 8e03dc0 to 7831ffe Compare August 22, 2017 18:23

Hari added 3 commits August 24, 2017 19:17

Add support for the UID EXPUNGE command

e08c764

Rewrite expunge when UIDPLUS is not available

1209d9b

Fix and add tests

1dcbaa7

rhari991 force-pushed the condstore-support branch 2 times, most recently from 33dc02c to 75a0452 Compare September 12, 2017 03:41

cketti closed this Jul 3, 2021

cketti deleted the rewrite-imap-idle branch January 28, 2023 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite IMAP IDLE #2671

Rewrite IMAP IDLE #2671

rhari991 commented Aug 9, 2017 •

edited

Loading

philipwhiuk commented Aug 13, 2017

Valodim Aug 13, 2017

rhari991 Aug 14, 2017

rhari991 Aug 18, 2017

Valodim left a comment

Valodim Aug 21, 2017

rhari991 Aug 21, 2017

rhari991 Aug 21, 2017

Valodim Aug 21, 2017

rhari991 Aug 21, 2017

Valodim Aug 22, 2017

rhari991 Aug 22, 2017

Valodim Aug 21, 2017

rhari991 Aug 21, 2017

Valodim Aug 21, 2017

rhari991 Aug 21, 2017

Valodim Aug 21, 2017

rhari991 Aug 21, 2017

lucafavatella commented Oct 7, 2017

philipwhiuk commented Oct 7, 2017


		folderPusher.start();

		verify(pushReceiver).syncFolder(FOLDER_NAME);

Rewrite IMAP IDLE #2671

Rewrite IMAP IDLE #2671

Conversation

rhari991 commented Aug 9, 2017 • edited Loading

philipwhiuk commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Valodim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucafavatella commented Oct 7, 2017

philipwhiuk commented Oct 7, 2017

rhari991 commented Aug 9, 2017 •

edited

Loading