nbook: a notmuch based address book written in python
Suvayu Ali
fatkasuvayu+linux at gmail.com
Tue Oct 16 07:55:03 PDT 2012
Hi Justus,
I finally had time to go through your response carefully.
On Mon, Oct 15, 2012 at 12:58:30PM +0200, Justus Winter wrote:
>
> > > > -------------------------------
> > > > [~] time nbook Patrick
> > > >
> > > > Error opening /home/pazz/mail/gmail/[Google Mail].All Mail/cur/1330682270_0.12958.megatron,U=8766,FMD5=66ff6a8bc18a8a3ac4b311daa93d358a:2,S: Too many open files
> > > > Traceback (most recent call last):
> > > > File "/home/pazz/bin/nbook", line 167, in <module>
> > > > File "/home/pazz/bin/nbook", line 71, in __init__
> > > > File "/home/pazz/.local/lib/python2.7/site-packages/notmuch/message.py", line 233, in get_header
> > > > notmuch.errors.NullPointerError
[...]
> > As mentioned before, I think you invalidate the Database object concurrently
> > while your long-running algorithm goes through all messages.
> > Xapian doesn't handle concurrent access to the index like a normal™ database would.
> > This means you are notified by this error that some changes were detected.
> > Maybe the error message should be more telling here though. Teythoon?
>
> The reason for this error is exactly what the error message says, you
> are opening to many files. Check out this limit using ulimit -n:
>
> % ulimit -n
> 4096
>
> This problem is subtle. Here is a minimal test case:
>
> ~~~ snip ~~~
> import notmuch
>
> with notmuch.Database() as db:
> query = notmuch.Query(db, 'a').search_messages()
> for msg in query:
> msg.get_header('from')
>
> with notmuch.Database() as db:
> query = notmuch.Query(db, 'a').search_messages()
> for msg in list(query):
> msg.get_header('from')
> ~~~ snap ~~~
>
> % python test.py
> Error opening /home/teythoon/Maildir/.lists.notmuch/cur/1323251462.M53044P18514.thinkbox,S=7306,W=7466:2,: Too many open files
> Traceback (most recent call last):
> File "test.py", line 11, in <module>
> msg.get_header('from')
> File "/home/teythoon/.local/lib/python2.7/site-packages/notmuch/message.py", line 237, in get_header
> raise NullPointerError()
> notmuch.errors.NullPointerError
>
> Observe that it blows up in line 11, the first version works. The only
> difference is that the second version creates a list from the notmuch
> query. This prevents the garbage collector from collecting the message
> objects and thus closing the file handles. So here's your fix:
>
> ~~~ snip ~~~
> diff --git a/nbook b/nbook
> index 387c71d..b3d4fd6 100755
> --- a/nbook
> +++ b/nbook
> @@ -173,7 +173,7 @@ class AddressHeaders(object):
> # Search
> db = Database()
> query = Query(db, 'from:"{0}" or to:"{0}"'.format(querystr))
> -msgs = list(query.search_messages())
> +msgs = query.search_messages()
>
> addresses = AddressHeaders(msgs, querystr)
> print addresses
> ~~~ snap ~~~
>
This explanation helped me a lot, thanks!
> A few more comments:
>
> > from notmuch import *
>
> Please avoid * imports, they prevent tools like pyflakes from checking
> whether you accidentally misspelled any identifiers.
>
Point taken. I'll be more careful in the future. :)
> > pyversion = float('%d.%d' % (sys.version_info.major, sys.version_info.minor))
> > if pyversion < 2.7:
>
> Converting this to float feels wrong. Consider doing sth like
>
> if sys.version_info.major > 2 or (sys.version_info.major == 2 and sys.version_info.minor >= 7):
>
I incorporated these suggestions too.
> > print '`nbook\' needs Python 2.7 or higher for argparse'
>
> Note that in py3k print is a function and not a statement, so you need
> to use braces. Consider dropping this at the beginning of all your
> python files to make py2.7 use the new features:
>
> from __future__ import print_function, absolute_import, unicode_literals
>
> > exit(-1)
>
> exit is not a builtin function. You have to use sys.exit. Tools like
> pyflakes can spot this kind of mistakes. Also, sys.exit also accepts a
> string as argument which it prints to stderr before exiting with an
> error code.
>
I will read-up some more about the above suggestions and update
accordingly.
> > self.__fromhdr__ += ',' + msg.get_header('from')
>
> Hm, this is somewhat unpythonic. It used to be the case that building
> strings this way was a lot slower than building a list and then
> joining it on a delimiter of your choice
> (i.e. ','.join(from_headers)). This is (was?) because strings are
> immutable in python and constantly creating strings just to throw them
> away in the next iteration puts a lot of pressure on the memory
> management system. Somewhat recent discussion here:
>
> http://stackoverflow.com/questions/1316887/what-is-the-most-efficient-string-concatenation-method-in-python
>
I had a commit with ','.join(..) in a private branch, but thanks for
pointing out the reasons and the links to the discussion. This was very
helpful.
> > def print_addrs(self, fmtstr='', query=''):
> > if '' == fmtstr: fmtstr = '%s %s\n'
>
> Ok, several things here:
>
> * The comparison looks weird, you are using the string constant as the
> first operand. While this is technically not wrong, it is somewhat
> unpythonic b/c if you read it out loud (''if the empty string is
> equal to fmtstr'') it somewhat bends the 1:1 mapping of the semantic
> of your program and the English sentence. It looks like this c hack
> that is actually unnecessary in python b/c you cannot use the
> assignment operator as a value (except for a=b=c=0 style
> assignments).
>
Yes you are correct, I'm more used to C/C++ and the reason you mention
is why I tend to write comparisons like that. I'll retrain my fingers
for python from now on.
> * Please don't put multiple statements in one line.
>
I will keep that in mind for the future.
> * This can be written shorter and more idiomatic (yay keyword
> arguments):
>
> def print_addrs(self, fmtstr='%s %s\n', query=''):
> [...]
>
That was silly of me not to do that in the first place! :-p
> Happy hacking :)
> Justus
Thank you soo much for this incredibly informative response. I learned
a lot.
Cheers,
--
Suvayu
Open source is the future. It sets us free.
More information about the notmuch
mailing list