Catch IMAP connection errors

[syntonym]

imaplib.IMAP4_SSL raises a socket.gaierror when something goes wrong with connecting
to PAPERLESS_CONSUME_MAIL_HOST. Later MailFetcherErrors are excepted, but socket.gaierror
are not, so instead raise a MailFetcherError. Because socket.gaierrors can have numerous
reasons (gaierror wraps EAI_* errors from getaddrinfo, so see man getaddrinfo EAI_*),
include the error description in the exception.

Fixes #474

[ovv]

This looks good 👍

Maybe we can catch all socket.* errors (socket.herror, socket.gaierror, socket.timeout).
See: https://docs.python.org/3/library/socket.html#exceptions

[syntonym]

I think these should never be raised by imaplib. socket.herror can be raised by gethostbyaddr() i.e. ip to host name lookup, which I don't think should happen for IMAP. socket.timeout should only happen when the socket is put into timeout mode, which is not the default afaik.

From 3.3 on all these are subclasses of OSError maybe we should just catch that one? It might accidentally catch some more errors (see list of OSError sources) but I guess the correct behavior for paperless is to log the error and retry later, so that there is nothing we could do with the individual exceptions anyway.

[ovv]

I'm not sure what python version paperless aims to support. If >= 3.3 then OSError sounds like the best solution to me.

[syntonym]

I can't find any sources right now but I thought that virtually all linux distributions are on python3 >= 3.3. As far as I see python 3.0-3.3 is end of life, so assuming python >= 3.3 seems unproblematic to me.

[MasterofJOKers]

Additionally, django = "<2.1,>=2.0" is a dependency. Django 2.0 supports 3.4, 3.5, 3.6, 3.7 (see FAQ). So even python >= 3.4 can be assumed.

[syntonym]

Should this include a test? I'm not super sure how to test that, besides spinning up a IMAP server and then manipulating the connection, which seems "overkill".

[MasterofJOKers]

You could use a Mock() object's side_effect attribute together with the patch decorator to monkeypatch imaplib.IMAP4_SSL to be a mock object.

[syntonym]

To me that seems to stray into "adding a test for adding a test" territory, but if you you it would be worth it I can add it.

[ovv]

I don't think a test is necessary for that. A wider check testing that any errors during the consumption doesn't crash it would be better IMO. But that's not really the scope of that PR.

[MasterofJOKers]

My point was just, that writing a test would be possible without an imap server. I agree that for this exception handler, probably no test is necessary, if we don't aim for 100% test coverage.

[danielquinn]

Honestly, I'd like to aim for 100% test coverage (or at least explicitly set #param nocover for code we don't want to worry about). As a friend of mine has recently brought me over to the 100% camp says, it makes for a good psychological barrier: dipping from 100% to 99.99% forces you to consider why it's ok not to cover this particular case, while dipping from 99.99% to 99.98% doesn't seem so bad.

With all that said though, I'm a pragmatist when it comes to this project and if there's consensus here, that this PR should be merged, then I'll hit the Big Green Button. @ovv, @MasterofJOKers, @syntonym, what say you?

[ovv]

I agree with you @danielquinn , tests would be better but we should also make sure that it doesn't drive contribution away. IMO a good starting point is to require tests for new features and important bugfixes. In this case I don't think a test would add anything.

[syntonym]

While test coverage is important I think even 100% of source lines covered by unit tests is "not enough". Adding a unit tests which throws a socket.gaierror is easy and improves test coverage, but a socket.timeout would still crash the consumer. This is even true with 100% test coverage (counted by lines covered by unit tests), so that even 100% test coverage doesn't guarantee anything.

As this PR is of low impact I'd advocate for merging it while I'm looking into writing some wider unittests covering the consumption process as ovv suggested.

[danielquinn]

Thanks for all the input guys. I'll merge this now.