@camillescott thank you for finding this and providing a fix

• minimal test for memory leak (doesn't have to be a nose test)

@mr-c: easiest way is to run:

import khmer
rp = khmer.ReadParser('large-paired-fasta.fa')
for pair in rp.iter_read_pairs():
    pass

Observe memory usage while it runs, and the leak will be obvious. Call del rp and nothing happens. Note that this is only an issue with the paired version, as it gives the Read references to a tuple before returning them.

I can confirm the memory and the fix

I get

terminate called after throwing an instance of 'khmer::read_parsers::NoMoreReadsAvailable'
  what():  Generic khmer exception
Aborted (core dumped)

After testing without the fix. This is inelegant, looking into that now.

ReadParser throws that exception when it runs out of reads, presumably to mimic a Python iterator (for reasons I'm not entirely sure of).

Is it only throwing that when multiple threads are in use? 'Cause it looks like the CPython iterator isn't thread-safe: there's a race condition there if another thread depletes the parser after checking is_complete(), which I believe will allow that exception to propagate.

Potential solution: catch the NoMoreReadsAvailable exception in the try block on line 409, which avoids another lock.

This was just the test code above. So single-threaded?

@camillescott and I have fixed the segfault that we inadvertently found due to a corrupted file in @ctb homedir.

Bonus: I've removed the unused callback code which has increased our code coverage.

• Is it mergable?
• Did it pass the tests?
• If it introduces new functionality in scripts/ is it tested?
  Check for code coverage.
• Is it well formatted? Look at make pep8, make diff_pylint_report,
  make cppcheck, and make doc output. Use make format and manual
  fixing as needed.
• Did it change the command-line interface? Only additions are allowed
  without a major version increment. Changing file formats also requires a
  major version number increment.
• Is it documented in the ChangeLog?
• Was a spellchecker run on the source code and documentation after
  changes were made?

 khmer._default_reporting_callback

 khmer._default_reporting_callback.info

 khmer._default_reporting_callback.n_reads

 khmer._default_reporting_callback.other

 khmer.reset_reporting_callback

 test_c_wrapper

 test_c_wrapper.callback_raise

 test_c_wrapper.callback_raise.info

 test_c_wrapper.callback_raise.n_reads

 test_c_wrapper.callback_raise.other

Showing only the first ten definitions. View the rest on ✱ sourcegraph.com

View the full smart diff on ✱ sourcegraph.com

Disable sourcegraphbot for this repository.

Not to rain on the code-axing parade, but some of that callback stuff is potentially useful for me.

I'll split that into another PR for a proper discussion.

which means I'll be force pushing to this branch in a moment, do beware!

branch force done.

Ready for review & merge @ctb @camillescott @luizirber

Looks good to me; merging.

Actually, I missed the wonky ChangeLog formatting. @mr-c, can this be fixed?