From: Dave M. <da...@an...> - 2011-12-04 15:24:01
Attachments:
noselect_fix.patch
|
Dear SquirrelMail Developers, I discovered that Squirrelmail 1.4.22 with $noselect_fix_enable set does not seem to be a complete fix, at least in a Cyrus 2.4.x environment. I'm not asserting that it would be a complete solution for earlier Cyrus versions (or for other IMAP servers, for that matter) but I've only tested it in a Cyrus 2.4.x environment. What I observed is that even with $noselect_fix_enable set to true, I had hierarchy levels (not mailboxes) being rendered in left_main.php as clickable links. If a user clicks on one of these links, SquirrelMail issues a SELECT to the IMAP server, and the server returns an error because NoSelect is set for that hierarchy level. I also discovered a similar issue when selecting "All Folders" from the search page. SquirrelMail would eventually attempt to SELECT a hierarchy level to search it and an error would be returned. The code at line 610 in functions/imap_mailbox.php that conditionally constructs the LSUB does the correct thing: if ($noselect_fix_enable) { $lsub_args = "LSUB \"$folder_prefix\" \"*%\""; } else { $lsub_args = "LSUB \"$folder_prefix\" \"*\""; } However, even if the NoSelect flag is returned in the LSUB response, it's thrown away when find_mailbox_name() is called at line 633. No later code expects to be able to glean NoSelect status from this LSUB response, so this isn't a problem. Instead, SquirrelMail calls LIST for each subscribed mailbox at line 662 to get all the mailbox flags. The problem I'm seeing is that the LIST isn't constructed similarly to the above LSUB and no data is returned at all for things like hierarchy levels that aren't mailboxes. Here's a protocol example using imtest against the cmu.misc level of hierarchy on our IMAP server: 1 LIST "" "cmu.misc" 1 OK Completed (0.000 secs 30 calls) 2 LIST "" "cmu.misc%" * LIST (\Noselect \NonExistent \HasChildren) "." cmu.misc 2 OK Completed (0.000 secs 30 calls) Attached is a patch that completes the $noselect_fix_enable fix in our environment. Can anyone else confirm whether or not $noselect_fix_enable works in different environments than ours? Let me know if you require any additional information. Thanks! Dave -- Dave McMurtrie, SPE Email Systems Technical Lead Carnegie Mellon University, Computing Services |
From: Paul L. <pa...@sq...> - 2011-12-06 11:53:51
Attachments:
mailbox_list_noselect_and_performance_fix.diff
|
Hi Dave, > I discovered that Squirrelmail 1.4.22 with $noselect_fix_enable set does not > seem to be a complete fix, at least in a Cyrus 2.4.x environment. I'm not > asserting that it would be a complete solution for earlier Cyrus versions > (or for other IMAP servers, for that matter) but I've only tested it in a > Cyrus 2.4.x environment. > > What I observed is that even with $noselect_fix_enable set to true, I had > hierarchy levels (not mailboxes) being rendered in left_main.php as > clickable links. If a user clicks on one of these links, SquirrelMail > issues a SELECT to the IMAP server, and the server returns an error because > NoSelect is set for that hierarchy level. I also discovered a similar issue > when selecting "All Folders" from the search page. SquirrelMail would > eventually attempt to SELECT a hierarchy level to search it and an error > would be returned. > > The code at line 610 in functions/imap_mailbox.php that conditionally > constructs the LSUB does the correct thing: > > if ($noselect_fix_enable) { > $lsub_args = "LSUB \"$folder_prefix\" \"*%\""; > } else { > $lsub_args = "LSUB \"$folder_prefix\" \"*\""; > } > > However, even if the NoSelect flag is returned in the LSUB response, it's > thrown away when find_mailbox_name() is called at line 633. No later code > expects to be able to glean NoSelect status from this LSUB response, so this > isn't a problem. Instead, SquirrelMail calls LIST for each subscribed > mailbox at line 662 to get all the mailbox flags. The problem I'm seeing is > that the LIST isn't constructed similarly to the above LSUB and no data is > returned at all for things like hierarchy levels that aren't mailboxes. > Here's a protocol example using imtest against the cmu.misc level of > hierarchy on our IMAP server: > > 1 LIST "" "cmu.misc" > 1 OK Completed (0.000 secs 30 calls) > 2 LIST "" "cmu.misc%" > * LIST (\Noselect \NonExistent \HasChildren) "." cmu.misc > 2 OK Completed (0.000 secs 30 calls) > > Attached is a patch that completes the $noselect_fix_enable fix in our > environment. Can anyone else confirm whether or not $noselect_fix_enable > works in different environments than ours? Thanks a lot for the thorough inquiry. It looks to me like this was unintentional, and you can in fact confirm that this was addressed in a similar manner in version 1.5.2. It's not clear if the code works for some people as-is, but I have my doubts (the spec and various implementations seem all too brittle here). While we're here, though, it occurs that doing one LIST per folder is egregious. Again, version 1.5.2 fixes this, too, and my tests show that the folder listing gains 11 to 12 times the performance by doing just a single LIST. So I'm prepared to add a slightly different patch, which is attached to this message. Note that it depends on another fix I've added to SVN just now which was a bug (and for you, Dave, you'll have to remove your patch as well). Here's the fix I just made: http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/functions/imap_mailbox.php?view=patch&r1=14175&r2=14174&pathrev=14175 Apply that, then the patch attached to this message and let me know if this works for you. Thanks again, -- Paul Lesniewski SquirrelMail Team Please support Open Source Software by donating to SquirrelMail! http://squirrelmail.org/donate_paul_lesniewski.php |
From: Dave M. <da...@an...> - 2011-12-06 16:22:23
|
On 12/06/2011 06:53 AM, Paul Lesniewski wrote: ...snipped... > While we're here, though, it occurs that doing one LIST per folder is > egregious. Again, version 1.5.2 fixes this, too, and my tests show > that the folder listing gains 11 to 12 times the performance by doing > just a single LIST. Yes, I see that 1.5.x handles this much better. > So I'm prepared to add a slightly different patch, which is attached > to this message. Note that it depends on another fix I've added to > SVN just now which was a bug (and for you, Dave, you'll have to remove > your patch as well). I removed my patch. > Here's the fix I just made: > > http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/functions/imap_mailbox.php?view=patch&r1=14175&r2=14174&pathrev=14175 I applied this patch. > Apply that, then the patch attached to this message and let me know if > this works for you. It doesn't appear to work. I'm getting clickable links for hierarchy levels again. I think the problem is that $temp_mailbox_name = find_mailbox_name($list_ary[$i]); just extracts the mailbox name and throws away all the flags, leaving you with a tidy array of just mailbox names. sqimap_mailbox_parse() seems to want the list output as-is so it can populate the 'raw' format array. |
From: Dave M. <da...@an...> - 2011-12-06 16:23:34
|
On 12/06/2011 11:22 AM, Dave McMurtrie wrote: > just extracts the mailbox name and throws away all the flags, leaving > you with a tidy array of just mailbox names. sqimap_mailbox_parse() > seems to want the list output as-is so it can populate the 'raw' format > array. ....and I hit Send by accident before I had a chance to actually try to determine why it's not working. Sorry about that :) I'll trace through it after lunch and send a more meaningful reply. |
From: Dave M. <da...@an...> - 2011-12-06 17:21:39
|
On 12/06/2011 06:53 AM, Paul Lesniewski wrote: ...snipped... > While we're here, though, it occurs that doing one LIST per folder is > egregious. Again, version 1.5.2 fixes this, too, and my tests show > that the folder listing gains 11 to 12 times the performance by doing > just a single LIST. > > So I'm prepared to add a slightly different patch, which is attached > to this message. Note that it depends on another fix I've added to > SVN just now which was a bug (and for you, Dave, you'll have to remove > your patch as well). > > Here's the fix I just made: > > http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/functions/imap_mailbox.php?view=patch&r1=14175&r2=14174&pathrev=14175 > > Apply that, then the patch attached to this message and let me know if > this works for you. Ignore my previous message. $force wasn't set, so Squirrelmail didn't update the folder list. Hence, I was still seeing the clickable links. Now that I started a new session, your patch appears to be working like a charm. Thanks much, Paul!! Dave |
From: Paul L. <pa...@sq...> - 2011-12-06 23:53:43
|
On Tue, Dec 6, 2011 at 9:21 AM, Dave McMurtrie <da...@an...> wrote: > On 12/06/2011 06:53 AM, Paul Lesniewski wrote: > ...snipped... >> While we're here, though, it occurs that doing one LIST per folder is >> egregious. Again, version 1.5.2 fixes this, too, and my tests show >> that the folder listing gains 11 to 12 times the performance by doing >> just a single LIST. >> >> So I'm prepared to add a slightly different patch, which is attached >> to this message. Note that it depends on another fix I've added to >> SVN just now which was a bug (and for you, Dave, you'll have to remove >> your patch as well). >> >> Here's the fix I just made: >> >> http://squirrelmail.svn.sourceforge.net/viewvc/squirrelmail/branches/SM-1_4-STABLE/squirrelmail/functions/imap_mailbox.php?view=patch&r1=14175&r2=14174&pathrev=14175 >> >> Apply that, then the patch attached to this message and let me know if >> this works for you. > > Ignore my previous message. $force wasn't set, so Squirrelmail didn't > update the folder list. Hence, I was still seeing the clickable links. > Now that I started a new session, your patch appears to be working > like a charm. Thanks for your feedback! -- Paul Lesniewski SquirrelMail Team Please support Open Source Software by donating to SquirrelMail! http://squirrelmail.org/donate_paul_lesniewski.php |