samtools / htslib

C library for high-throughput sequencing data formats
Other
799 stars 445 forks source link

bgzf_read_block added error return #1529

Closed bir3 closed 1 year ago

bir3 commented 1 year ago

fixed memory leak

whitwham commented 1 year ago

Confirmed that the memory leak has gone. Is PR ready for review and merging?

bir3 commented 1 year ago

Ready for review, it would be good to get experienced eyes on this.

Not sure about desired semantics of single-thread vs. multi-thread in the presence of a truncated file:

The fix as written adheres to agreeing with the single-threaded behaviour, e.g. it will return all valid blocks until it hits the truncated block. The test is however relaxed and allows both cases. So it could be tightened if the decision is to favor agreeing with single-threaded behaviour.

We might also want to add test for EOF block in the presence of a truncated file.

whitwham commented 1 year ago

Not sure about desired semantics of single-thread vs. multi-thread in the presence of a truncated file

We had a quick discussion here and I think we are happy to have the multi-threaded run return an error early. We don't really need identical behaviour between single and multi-threaded on a failure.

bir3 commented 1 year ago

Changed to exit early on error - as the code did before. Ready for review and merging if deemed acceptable.

whitwham commented 1 year ago

Thank you for your fix.