Improve DKIM performance

[benmezger]

This PR improves DKIM performance. It runs the filter asynchronously by adding the messages to a coroutine and finally gathering all messages

[codecov[bot]]

Codecov Report

Merging #316 (61b26dc) into master (8ef9a5b) will increase coverage by 1.85%. The diff coverage is 95.87%.

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   47.08%   48.93%   +1.85%     
==========================================
  Files          30       30              
  Lines        1079     1126      +47     
==========================================
+ Hits          508      551      +43     
- Misses        571      575       +4     

Flag	Coverage Δ
unittests	48.93% <95.87%> (+1.85%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
afew/main.py	0.00% <0.00%> (ø)
afew/filters/DKIMValidityFilter.py	100.00% <100.00%> (ø)
afew/tests/test_dkimvalidityfilter.py	100.00% <100.00%> (ø)
afew/filters/HeaderMatchingFilter.py	40.00% <0.00%> (-2.11%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8ef9a5b...61b26dc. Read the comment docs.

[GuillaumeSeren]

Hello @benmezger, thank you for this patch.

I will need some time to review it, but at least it already have some tests, but I can see a lot of 'not covered by tests' if you want to add them ?

Maybe @flokli want to also spend a review ?

[benmezger]

@GuillaumeSeren thanks. b2d7f22 fixes the coverage.

[GuillaumeSeren]

Hey @benmezger , thank for the feedback.

Now the coverage is fixed but some tests fail (see python3.7) can you check ?

[benmezger]

Now the coverage is fixed but some tests fail (see python3.7) can you check ?

Maybe I forgot to run the test against 3.7. Should be working now.

[benmezger]

@GuillaumeSeren do you mind enabling the actions to run on every PR? it seems that in order for the action to run, someone needs to give permission.

[GuillaumeSeren]

Hey @benmezger thank you for this work, and sorry for the delay.

I checked the action permission settings it is actually 'allow all actions' is that what you are talking about ?

Also this branch is growing and it would be great to be able to merge it, but as it become wider it need's a bit more time to review, so please be patient.

So now all the tests are working, that's a good starting point !

I like that it is not a complete refactor but a optimisation, so user's that doesn't want (or can't) have this dependency, can't still be fine and keep their filter running.

Maybe @aidecoe is able to help us review this as he created this filter.

[benmezger]

I checked the action permission settings it is actually 'allow all actions' is that what you are talking about ?

It seems that every time I push something to this PR, the action does not run and I have to wait for admin approval. Next time I push something I will check what is going on.

I like that it is not a complete refactor but a optimisation, so user's that doesn't want (or can't) have this dependency, can't still be fine and keep their filter running.

Exactly. It is interesting to run asynchronous as it would speed up the execution time when possible. I cannot think of any limitation as of now, though.

I have been merging these changes to my own fork of this repository (https://github.com/benmezger/afew) and have been recently working on a small configuration refactor in order to support TOML configuration format. The reason I am adding support to TOML is that I am trying to to map directories 1:1 on the MailMover including tagging/untagging, but for this, I had to port a different configuration format to make things easier to handle. Its still a WIP, though. https://github.com/benmezger/afew/tree/feat/add-toml-support

-- Ben Mezger

https://seds.nl https://github.com/benmezger

[GuillaumeSeren]

Hey @benmezger , I think the ci issue is fixed now.

I think this patch is good and I want to approve it.

It feel safe to merge because it does not break synchronous work, so it should be only like a testing / performance feature for those who want it.

I'll wait a little bit longer to let a chance for more review but I think it going well.

[aidecoe]

@benmezger could you share the benchmark results before and after, please?

[aidecoe]

@benmezger could you explain how this is actually running asynchronously?

To my (perhaps wrong) understanding, the code needs to support async down to the I/O operations. Async is not threads, it's more of I/O multiplexing. So unless python-notmuch supports it, I don't understand how we could benefit from this change at all.