Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IMAP PARTIAL and INPROGRESS support #4716

Merged
merged 13 commits into from
Jan 5, 2024
Merged

IMAP PARTIAL and INPROGRESS support #4716

merged 13 commits into from
Jan 5, 2024

Conversation

ksmurchison
Copy link
Contributor

@ksmurchison ksmurchison commented Nov 3, 2023

This PR mainly adds support for IMAP PARTIAL and INPROGRESS extensions but also does some code reorganization and adds an optimization for SEARCH with a sequence set at the toplevel of the criteria.

This PR is best reviewed commit-by-commit

@ksmurchison ksmurchison marked this pull request as draft November 3, 2023 14:46
@ksmurchison ksmurchison force-pushed the imap-partial branch 2 times, most recently from 7fd5c90 to 5c76fa7 Compare November 13, 2023 17:27
@ksmurchison ksmurchison force-pushed the imap-partial branch 3 times, most recently from a046434 to 9bd2257 Compare December 6, 2023 17:30
@ksmurchison ksmurchison changed the title Start of IMAP PARTIAL support IMAP PARTIAL support Dec 6, 2023
@ksmurchison ksmurchison marked this pull request as ready for review December 6, 2023 20:11
@ksmurchison ksmurchison requested review from rsto and elliefm December 6, 2023 20:13
Copy link
Contributor

@elliefm elliefm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI failure is from JMAPEmail.email_query_guidsearch_mixedfilter, in which httpd is crashing. We don't get core files on CI, but I was able to reproduce it locally. The backtrace is below. Running this test under valgrind finds a few memory errors before the crash.

#0  0x00007f067580c13e in index_reload_record (state=0x55652d5018b0, msgno=2063758341, 
    record=0x7ffe0a29bba0) at imap/index.c:209
209	    if (!im->recno) {
(gdb) bt
#0  0x00007f067580c13e in index_reload_record (state=0x55652d5018b0, msgno=2063758341, 
    record=0x7ffe0a29bba0) at imap/index.c:209
#1  0x00007f067581d2b4 in index_msgdata_load (state=0x55652d5018b0, 
    msgno_list=0x55652d5059d0, n=1, sortcrit=0x55652d4fce70, anchor=0, found_anchor=0x0)
    at imap/index.c:6286
#2  0x00007f067588b7ef in query_load_msgdata (query=0x55652d509660, 
    folder=0x55652d505590, state=0x55652d5018b0, msgno_list=0x55652d5059d0, nmsgs=1)
    at imap/search_query.c:454
#3  0x00007f067588bee6 in _subquery_run_one_folder (query=0x55652d509660, qr=0x0, 
    folder=0x55652d505590, mboxname=0x55652d501e00 "user.cassandane.INBOX.B", 
    e=0x55652d505850) at imap/search_query.c:642
#4  0x00007f067588cc29 in subquery_run_one_folder (query=0x55652d509660, 
    mboxname=0x55652d501e00 "user.cassandane.INBOX.B", e=0x55652d505850)
    at imap/search_query.c:947
#5  0x00007f067588ccd3 in subquery_run_folder (
    key=0x55652d501e00 "user.cassandane.INBOX.B", data=0x55652d509380, 
    rock=0x55652d509660) at imap/search_query.c:960
#6  0x00007f067552186d in hash_enumerate (table=0x55652d5096d8, 
    func=0x7f067588cc3f <subquery_run_folder>, rock=0x55652d509660) at lib/hash.c:340
#7  0x00007f067588d23f in search_query_run (query=0x55652d509660)
    at imap/search_query.c:1098
#8  0x000055652c1915be in emailsearch_run_uidsearch (req=0x7ffe0a29c6b0, 
    search=0x7ffe0a29c0f0) at imap/jmap_mail.c:3019
#9  0x000055652c1949a3 in emailquery_uidsearch (req=0x7ffe0a29c6b0, q=0x7ffe0a29c360, 
    search=0x7ffe0a29c0f0, qr=0x55652c23bda0 <emailquery_cache>, err=0x7ffe0a29c2c0)
    at imap/jmap_mail.c:4189
#10 0x000055652c194e2d in emailquery_search (req=0x7ffe0a29c6b0, q=0x7ffe0a29c360, 
    qr=0x55652c23bda0 <emailquery_cache>, contactgroups=0x7ffe0a29c338, 
    errp=0x7ffe0a29c2c0) at imap/jmap_mail.c:4284
#11 0x000055652c19647f in emailquery_run (req=0x7ffe0a29c6b0, q=0x7ffe0a29c360, 
    contactgroups=0x7ffe0a29c338, errp=0x7ffe0a29c2c0) at imap/jmap_mail.c:4655
--Type <RET> for more, q to quit, c to continue without paging--c
#12 0x000055652c196ef1 in jmap_email_query (req=0x7ffe0a29c6b0) at imap/jmap_mail.c:4811
#13 0x000055652c11ab64 in jmap_api (txn=0x7ffe0a29c9b0, jreq=0x55652d50b5c0, 
    res=0x7ffe0a29c8e0, settings=0x55652c23bbe0 <my_jmap_settings>)
    at imap/jmap_api.c:852
#14 0x000055652c11305e in meth_post (txn=0x7ffe0a29c9b0, params=0x0)
    at imap/http_jmap.c:518
#15 0x000055652c0db129 in process_request (txn=0x7ffe0a29c9b0) at imap/httpd.c:1981
#16 0x000055652c0db767 in http1_input (txn=0x7ffe0a29c9b0) at imap/httpd.c:2118
#17 0x000055652c0dbaea in cmdloop (conn=0x55652c234b60 <http_conn>) at imap/httpd.c:2224
#18 0x000055652c0d87e5 in service_main (argc=1, argv=0x55652d44bef0, envp=0x7ffe0a2a17a8)
    at imap/httpd.c:1074
#19 0x000055652c10ee7d in main (argc=3, argv=0x7ffe0a2a1788, envp=0x7ffe0a2a17a8)
    at master/service.c:647

lib/imparse.c Show resolved Hide resolved
lib/imparse.c Outdated Show resolved Hide resolved
lib/imparse.c Outdated Show resolved Hide resolved
@ksmurchison
Copy link
Contributor Author

Crasher fixed

@ksmurchison ksmurchison requested a review from elliefm December 9, 2023 21:46
Copy link
Contributor

@elliefm elliefm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Couple of typos that need fixing, but doesn't need reviewing again

cunit/imparse.testc Outdated Show resolved Hide resolved
lib/imparse.c Outdated Show resolved Hide resolved
@ksmurchison ksmurchison force-pushed the imap-partial branch 3 times, most recently from e28457c to 164baea Compare December 11, 2023 18:15
@ksmurchison ksmurchison changed the title IMAP PARTIAL support IMAP PARTIAL and INPROGRESS support Dec 16, 2023
@ksmurchison ksmurchison requested a review from elliefm December 16, 2023 17:24
@elliefm
Copy link
Contributor

elliefm commented Dec 18, 2023

InProgress.copy_search_slow fails locally for me, for a surprising reason...

C: 11 RENAME INBOX INBOX.Archive
S: * OK [INPROGRESS ("11" 3962 15000)] Still processing...
S: 11 NO System I/O error
IOERROR: retry_write failed: filename=</dev/shm/cass/0046080101/data/user/cassandane/Archive/4440.> syserror=<No space left on device> func=<_copyfile_helper>

I have my cassandane set to put all its data on tmpfs (under /dev/shm/cass) so that it doesn't get slowed down by real disk I/O. But 15k_messages.tar.gz unpacks to 1.7GB of files in INBOX, which then gets duplicated into INBOX.dst, and then the rename is trying to duplicate it all again (I think?), and my tmpfs just doesn't have space for it.

The message files are each 116KB, which is a basic cassandane test message followed by 5k lines of "This is an extra line". Do they need to be so large?

@ksmurchison
Copy link
Contributor Author

All I know is that the search needs to be slow enough to spit out an INPROGRESS repsonse. Fortunately our stuff is really fast and slowing it down is hard to do

@elliefm
Copy link
Contributor

elliefm commented Dec 18, 2023

Heh yeah. Makes me think it might be useful to have some kind of debug_artificial_delay imapd.conf option, which when enabled, makes things like append, search, etc sleep() every so often. Then we'd be able to test this sort of thing without needing a huge volume of data/messages to force a slowdown -- just make the test set that config option. We already have similar things in debug_log_sync_partition_choice and debug_writefail_guid, in that they're config options that exist so tests can force weird conditions you'd never want IRL.

@elliefm
Copy link
Contributor

elliefm commented Dec 21, 2023

Makes me think it might be useful to have some kind of debug_artificial_delay imapd.conf option, which when enabled, makes things like append, search, etc sleep() every so often.

I had a hit of inspiration about this idea overnight, so I'm going to have a crack at implementing something like this today. If it works out, I'll eventually update the copy_search_slow test to use it and shrink the data file so that the test can pass on a storage-limited environment.

If you've already started doing something similar, let me know; or if I make a bunch of progress before hearing from you, we can compare implementations next year.

@ksmurchison
Copy link
Contributor Author

Have at it. I haven't touched the code since the last bug fix. Been reading a Rust book all week

@ksmurchison ksmurchison merged commit 9a89959 into master Jan 5, 2024
2 checks passed
@ksmurchison ksmurchison deleted the imap-partial branch January 5, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants