incomplete error handling

[brentp]

hi, I'm using SeqLib via freebayes. I just unmounted the file system holding the bams that freebayes was accessing. but the program still had a 0 exit status.

I see this message: [E::bgzf_read] bgzf_read_block error -1 after 1115 of 1202 bytes which is correctly propagated by htslib (AFAICT).

My C++ is rusty/non-existent, but it looks like _Bam::load_read(BamRecord& r) in SeqLib is just using a non-zero return from sam_read1 as an indicator to end iteration without actually propagating the error.

So it appears that the iterator just stops after getting the -1 return value from sam_read1 and can never error. Am I mistaken?

Here is the code in question: https://github.com/walaj/SeqLib/blob/770cd10c308430e1719d54ebedcfe708db560bec/src/BamReader.cpp#L305-L355

[walaj]

This is a good suggestion. The Open function for opening the BAM should return false if the BAM is unmounted, but I guess in your case if it's unmounted during the middle of the run, then I agree this should give a run-time error.

Looking over the htslib code, it looks like -1 is returned by sam_read1 for normal EOF, so really load_read should throw an error for values of sam_read1 of -2 or less. This should give a non-zero exit status in your situtation, unless the user wants to catch this error.

I can make the adjustment and check in. I think I still have a pending PR on Freebayes from November, so can add to that. I can re-ping Erik and see if he wants to incorporate.

[brentp]

I think you're right that -1 is EOF and other negative values are errors. That would be great to get this into SeqLib and then freebayes. We're using it in places where network failures can remove drive access and we need to know this happened.

[walaj]

Hi Brent,

I fixed this in the latest SeqLib commit. I didn't update my FreeBayes PR, since there are already a number of adjustments and it was getting to be too much in one pass. If you want, you can just update the SeqLib sub-repos to the latest SeqLib commit in your freebayes directory, and rebuild to have this issue fixed.

After my latest freebayes PR goes through, I can issue another one to update the official freebayes version.

Testing after force-ejecting a volume, confirms an exit code of 134 with the updated SeqLib

[E::bgzf_read] bgzf_read_block error -1 after 0 of 4 bytes
status -2 
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: sam_read1 return status: -2 file: /Volumes/jwala/tmptmp.bam
Abort trap: 6

wmbb0-9c3:freebayes jwala$ echo $?
134

[brentp]

sweet! thanks for the quick fix.

[brentp]

Is the pending change to freebayes this one: https://github.com/ekg/freebayes/pull/338

If so, once that PR is up-to-date, we should ping that issue as this is fairly critical.