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.

Sign up for GitHub

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

Jump to bottom

squatter: do not hold mailbox locks while calling the attachment extract service #4480

Merged
merged 23 commits into from
Jul 26, 2023
Merged

squatter: do not hold mailbox locks while calling the attachment extract service #4480

merged 23 commits into from
Jul 26, 2023

Conversation

rsto
Copy link
Member

@rsto rsto commented Apr 17, 2023 •
edited
Loading

This updates squatter to not lock a mailbox while extracting text from attachments. To do so, the attachment extraction module now supports an optional file-system based cache. By default, squatter uses a temporary cache. The newly added experimental squatter argument options v--attachextract-cache-dir and --attachextract-cache-only allow to use user-defined cache directory, as well as instruct squatter to only lookup extracted attachment text in the cache.

In addition, handling of timeouts when calling the extractor service has changed: rather than attempting to connect up to 4 times, an error now is returned after the first timeout.

This pull request best is read commit by commit, where the first two commits are refactoring only.

@rsto rsto changed the title Do not hold mailbox locks while calling the attachment extract service squatter: do not hold mailbox locks while calling the attachment extract service Apr 17, 2023
ksmurchison
ksmurchison reviewed Apr 18, 2023
View reviewed changes
Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

I don't know a ton about the extractor, but most of this appears to be sane.
I still don't understand with the port_waitevent is used for.

@rsto
Copy link
Member Author

rsto commented Apr 18, 2023

@ksmurchison the comment in this commit indicates that the prot_waitevent timeout is used to close network connections to the backend if it hasn't been used recently. See here: https://github.com/cyrusimap/cyrus-imapd/blame/f0f8a130b889381da1beb89962727f855370a301/imap/index.c#L5590
I guess we could achieve the same by adding the waitevent on the http client out stream, rather than clientin?

@ksmurchison
Copy link
Contributor

@ksmurchison the comment in this commit indicates that the prot_waitevent timeout is used to close network connections to the backend if it hasn't been used recently. See here: https://github.com/cyrusimap/cyrus-imapd/blame/f0f8a130b889381da1beb89962727f855370a301/imap/index.c#L5590
I guess we could achieve the same by adding the waitevent on the http client out stream, rather than clientin?

No, this is fine. After thinking about it, this is he same thing that we do with the backend IMAP connections in a Murder.

elliefm
elliefm requested changes Apr 19, 2023
View reviewed changes
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.

Bunch of comments and suggestions, some just nits, some more important.

cassandane/Cassandane/Cyrus/SearchFuzzy.pm Outdated Show resolved Hide resolved
cassandane/Cassandane/Cyrus/SearchFuzzy.pm Outdated Show resolved Hide resolved
cassandane/Cassandane/Cyrus/SearchFuzzy.pm Outdated Show resolved Hide resolved
cassandane/Cassandane/Cyrus/SearchFuzzy.pm Outdated Show resolved Hide resolved
cassandane/Cassandane/Cyrus/SearchFuzzy.pm Outdated Show resolved Hide resolved
imap/attachextract.c Show resolved Hide resolved
imap/attachextract.c Outdated Show resolved Hide resolved
imap/search_engines.c Outdated Show resolved Hide resolved
imap/search_engines.c Outdated Show resolved Hide resolved
cassandane/Cassandane/Cyrus/SearchFuzzy.pm Outdated Show resolved Hide resolved
@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from 72ae24b to f731de2 Compare April 20, 2023 11:55
@rsto
Copy link
Member Author

rsto commented Apr 20, 2023

@elliefm thanks, I have updated the code accordingly

@rsto rsto requested a review from elliefm April 20, 2023 11:55
elliefm
elliefm reviewed Apr 21, 2023
View reviewed changes
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.

Weird stuff still going on with that one chmod test... I've attached a patch with some extra debug logging that identifies exactly when things get weird, but I have no idea why they get weird.

Also got a failure from SearchFuzzy.squatter_attachextract_nolock_slow but I haven't been able to reproduce it, it started passing as soon as I started looking at it.

imap/index.c Show resolved Hide resolved
cassandane/Cassandane/Cyrus/SearchFuzzy.pm Outdated Show resolved Hide resolved
cassandane/Cassandane/Cyrus/SearchFuzzy.pm Outdated Show resolved Hide resolved
@rsto rsto requested a review from elliefm April 24, 2023 16:15
@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from f731de2 to a20be29 Compare April 26, 2023 06:03
@rsto
Copy link
Member Author

rsto commented May 2, 2023

Tagging this Do-Not-Merge for now: I first want to get #4495 into the codebase.

@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from a20be29 to 5dde1ee Compare May 15, 2023 13:49
@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from 5dde1ee to 6e84d70 Compare May 15, 2023 16:18
@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from 6e84d70 to 39ce3dc Compare May 19, 2023 09:18
@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from 39ce3dc to 8cd385b Compare May 23, 2023 08:13
@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from f8d81fb to 5110853 Compare June 7, 2023 13:46
@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from 121516b to db2b787 Compare June 16, 2023 11:28
@rsto rsto removed the Do Not Merge label Jun 19, 2023
rsto added 23 commits July 26, 2023 13:23
@rsto
attachextract: refactor attachment text extractor from index.c
1731d30 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: split extract arguments in content and metadata
380885c 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
message_guid: add clone function as alternative to copy
101b589 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
cassandane: add NoCheckSyslog magic to omit syslog error checks
fa171d1 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
dynarray: make initializing dynarrays nicer
8230fc1 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
strarray: add helpers to return newly inserted value
d72ae3b 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: optionally cache extracted attachment text on disk
d865ebf 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
TestCase: add strictly less-than and greater-than asserts
6181dfa 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
squatter: release mailbox lock while extracting attachments
ba7fc3a 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: add options to handle timeout and idle connections
0646729 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: reset backend if GET request times out
2413023 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: fix jump-over-init error
caa7e80 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: log error from HTTP client before resetting it
8c45f9a 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: log error when HTTP request could not be sent
0fd4718 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: reconnect to extractor if connection expired
7f639d8 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: do not pass charset parameter to extractor
a35219e 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
index: update list of unwanted attachment mime types
6a2b42a 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: make syslogs less noisy
d7be3a7 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: add debug log when reusing open socket
008c834 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: retry sending HTTP request on error
60a2555 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: reset extractor connection on any error
d5a03ff 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
squatter: do not index JMAP upload and notification mailboxes
485bd96 
Signed-off-by: Robert Stepanek <[email protected]>
@rsto
attachextract: do not handle unprocessable content as error
406fa26 
This fixes a regression where the HTTP 422 response code of
the attachment extractor service was inadvertently handled as
an error. As a result, the message was tagged as partially
indexed.

Signed-off-by: Robert Stepanek <[email protected]>
@rsto rsto force-pushed the squatter_nolock_attachment_text_extraction branch from db2b787 to 406fa26 Compare July 26, 2023 11:26
@rsto rsto merged commit 789d955 into master Jul 26, 2023
2 checks passed
@rsto rsto deleted the squatter_nolock_attachment_text_extraction branch July 26, 2023 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ksmurchison ksmurchison ksmurchison approved these changes

@elliefm elliefm elliefm approved these changes

@brong brong Awaiting requested review from brong

Assignees
No one assigned
Labels
include-in-fastmail
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rsto @ksmurchison @elliefm