Implement unsolicited message handling while IDLEing #130

norcalli · 2019-08-04T17:36:28Z

Adapted from #114

This change is

norcalli · 2019-08-04T18:13:56Z

Fixes #128

norcalli · 2019-08-04T18:15:30Z

I had to change around the logic for the "read_response" after "terminate" because there could've been multiple messages sent from the server, so we need to iterate over all of them, but I also need to know when to stop, and the only cue to stop is the OK we get from the server for the IDLE DONE.

norcalli · 2019-08-04T18:17:32Z

I'm also not sure about making the unsolicited responses be empty by default, because I found it surprising. However, from an engineering perspective, it makes sense because if someone doesn't handle them then they build up in memory. This could be solved by a bounded channel with a sane default, as well, to impose an upper limit.

For now, I think dumping all unsolicited responses unless otherwise requested is easier and works fine, though.

norcalli · 2019-08-04T19:04:22Z

The test failure appears to be unrelated to any code I wrote.

norcalli · 2019-08-05T02:16:57Z

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `[97, 49, 56]`,
 right: `[97, 49, 57]`', /home/ashkan/works/3rd/rust-imap/src/client.rs:1206:25
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

had an error related to match being one shorter than the client expected. I'm wondering if me doing the matching for the ok response in the handle is breaking some invariants.

norcalli · 2019-08-11T20:48:29Z

FYI, I've been running this for the past week without any crashes or problems. I'm using it as my main ingestion pipeline for my mail now.

jonhoo · 2019-08-21T19:36:25Z

I've been away on vacation for the past few weeks, so haven't had a chance to look. Will try to take a look once I get the time! Thanks for writing this up! :D

jonhoo · 2019-08-21T19:41:15Z

src/parse.rs

@@ -46,7 +46,7 @@ where
                    match map(resp)? {
                        MapOrNot::Map(t) => things.push(t),
                        MapOrNot::Not(resp) => match handle_unilateral(resp, unsolicited) {
-                            Some(Response::Fetch(..)) => continue,
+                            // Some(Response::Fetch(..)) => continue,


Why this change?

jonhoo · 2019-08-21T19:42:13Z

src/extensions/idle.rs

+                    }
+                }
+                Err(err) => return Err(err),
+                Ok(_) => {


I don't understand this clause at all. Why do we need this nested loop construct? And why do we need to terminate the IDLE for each iteration?

jonhoo · 2019-08-21T19:42:39Z

src/parse.rs

@@ -202,7 +199,7 @@ pub fn parse_mailbox(
                if let imap_proto::Status::Ok = status {
                } else {
                    // how can this happen for a Response::Data?
-                    unreachable!();
+                    unreachable!("Received something other than OK from mailbox data");


This should probably include status for convenience.

jonhoo · 2019-08-21T19:43:27Z

src/parse.rs

+                        return Err(Error::Bad(information.unwrap_or("").to_owned()))
+                    }
+                    imap_proto::Status::No => {
+                        return Err(Error::Bad(information.unwrap_or("").to_owned()))


Don't we also have Error::No?

jonhoo · 2019-08-21T19:44:40Z

src/extensions/idle.rs

+                }
+                Err(err) => return Err(err),
+                Ok(_) => {
+                    let _ = parse::parse_idle(&buffer, &mut self.unsolicited_responses_tx)?;


Specifically, shouldn't this use the return value from parse_idle to know whether or not DONE was sent?

jonhoo · 2019-08-21T19:46:00Z

src/parse.rs

+            }
+            Ok((rest, data)) => {
+                lines = rest;
+                if let Some(resp) = handle_unilateral(data, unsolicited) {


We should probably rename handle_unilateral to try_handle_unilateral to make it clearer that Some is returned if it wasn't handled. That's orthogonal to your change though :)

jonhoo · 2019-08-21T19:47:41Z

src/types/mod.rs

+    Unseen(u32),
+}
+
+impl<'a> From<imap_proto::types::ResponseCode<'a>> for ResponseCode {


I wonder if ToOwned is the trait to implement here.

jonhoo · 2019-08-21T19:50:50Z

src/unsolicited_responses.rs

+    // Set the new filter mask, and remove unwanted responses from the current queue.
+    pub(crate) fn request(
+        &mut self,
+        receiver: &mpsc::Receiver<UnsolicitedResponse>,


I'm not sure I understand what this receiver is? Where does it come from?

jonhoo · 2019-08-21T19:51:08Z

src/unsolicited_responses.rs

+        mask: EnumSet<UnsolicitedResponseCategory>,
+    ) {
+        self.allow = mask;
+        for message in receiver.try_iter() {


This also seems pretty strange -- what about future messages? Are they never forwarded then?

I copied a lot of stuff from the existing branch, assuming that it was already reviewed. About 20% of it is mine. Frankly, I might rewrite it. I've noticed that some messages are missed if they come in too quickly.

Long time no see :) Did you ever get around to doing this rewrite?

jonhoo · 2019-09-01T21:58:45Z

Hi @norcalli -- a gentle ping on this :)

norcalli · 2019-09-11T19:43:00Z

@jonhoo hey sorry for the late reply, gonna take a look at this today.

hpk42 · 2019-10-02T13:45:47Z

@norcalli just so you i'd be interested in your PR landing but my Rust-Fu is not good enough yet to help much.

jonhoo · 2021-01-29T01:24:27Z

@norcalli Another gentle ping on this :)

jonhoo · 2021-03-06T23:26:32Z

@norcalli I'd like to try to push out 3.0.0 soon — want to give this another stab?

jonhoo · 2021-04-20T02:30:44Z

Replaced by #186.

Ashkan Kiani added 3 commits August 4, 2019 10:34

Implement unsolicited message handling while IDLEing

725ec9d

Adapted from jonhoo#114

Bugfix response reading.

800086d

Forgot a debug print

adee3f1

Bugfix idle refresh out of order responses.

f17b494

jonhoo reviewed Aug 21, 2019

View reviewed changes

jonhoo mentioned this pull request Aug 22, 2019

Implemented new enumerated Capabilities from imap-proto crate. #120

Closed

This was referenced Sep 16, 2019

SSL/TLS dependency #123

Closed

release new version #141

Closed

jonhoo mentioned this pull request May 18, 2020

Implemented iteration over unsolicited responses during IDLE. #114

Closed

jonhoo added this to the 3.0.0 milestone Dec 15, 2020

mordak mentioned this pull request Mar 20, 2021

Try handle unsolicited & clippy #184

Merged

mordak mentioned this pull request Mar 28, 2021

Pass IDLE responses to caller. #186

Merged

jonhoo closed this Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unsolicited message handling while IDLEing #130

Implement unsolicited message handling while IDLEing #130

norcalli commented Aug 4, 2019 •

edited by jonhoo

Loading

norcalli commented Aug 4, 2019

norcalli commented Aug 4, 2019

norcalli commented Aug 4, 2019 •

edited

Loading

norcalli commented Aug 4, 2019

norcalli commented Aug 5, 2019

norcalli commented Aug 11, 2019

jonhoo commented Aug 21, 2019

jonhoo Aug 21, 2019

jonhoo Aug 21, 2019

jonhoo Aug 21, 2019

jonhoo Aug 21, 2019

jonhoo Aug 21, 2019

jonhoo Aug 21, 2019

jonhoo Aug 21, 2019

jonhoo Aug 21, 2019

jonhoo Aug 21, 2019

norcalli Sep 12, 2019

jonhoo Dec 15, 2020

jonhoo commented Sep 1, 2019

norcalli commented Sep 11, 2019

hpk42 commented Oct 2, 2019

jonhoo commented Jan 29, 2021

jonhoo commented Mar 6, 2021

jonhoo commented Apr 20, 2021

Implement unsolicited message handling while IDLEing #130

Implement unsolicited message handling while IDLEing #130

Conversation

norcalli commented Aug 4, 2019 • edited by jonhoo Loading

norcalli commented Aug 4, 2019

norcalli commented Aug 4, 2019

norcalli commented Aug 4, 2019 • edited Loading

norcalli commented Aug 4, 2019

norcalli commented Aug 5, 2019

norcalli commented Aug 11, 2019

jonhoo commented Aug 21, 2019

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

jonhoo commented Sep 1, 2019

norcalli commented Sep 11, 2019

hpk42 commented Oct 2, 2019

jonhoo commented Jan 29, 2021

jonhoo commented Mar 6, 2021

jonhoo commented Apr 20, 2021

norcalli commented Aug 4, 2019 •

edited by jonhoo

Loading

norcalli commented Aug 4, 2019 •

edited

Loading