Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure all sam_read1() and sam_itr_next() failures are reported #1379

Merged
merged 2 commits into from Feb 22, 2021

Conversation

jmarshall
Copy link
Member

@jmarshall jmarshall commented Feb 17, 2021

Ensure that error return codes from all invocations of these functions are captured and result in a diagnostic message and failing exit status. An audit of the 32 instances of sam_read1() and the 10 instances of sam_itr_next() in code contributing to the samtools executable showed that the following were missing and are fixed by this PR:

For coverage, depth, fixmate, mpileup: the command already exited with failure but this adds an error message. (For mpileup, this changes the exit status in this case from 255 (i.e., -1 🤮) to 1.)

For flagstat: convert a "Continue anyway" message into an error and failure.

For bedcov, phase, targetcut: add the missing bam_[m]plp_auto() check and error message and failure.

For tview: add the missing sam_itr_next() check, and exit immediately (as tview already does for other "can't happen" errors.)

Fixes #101. Fixes a number of instances of #51.

Ensure that error return codes from all invocations of these functions
are captured and result in a diagnostic message and failing exit status.

For coverage, depth, fixmate, mpileup: the command already exited with
failure but this adds an error message.

For flagstat: convert a "Continue anyway" message into an error and
failure.

For bedcov, phase, targetcut: add the missing bam_[m]plp_auto() check
and error message and failure.

For tview: add the missing sam_itr_next() check, and exit immediately
(as tview already does for other "can't happen" errors.)
Copy link
Member

@daviesrob daviesrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, these are certainly long overdue.

Some of them seem to keep the "continue anyway" behaviour, albeit with returning an error code now. I think it might be time to make them bail out as soon as the error is found, as the output is unlikely to be useful anyway.

bam2depth.c Outdated Show resolved Hide resolved
bam_plcmd.c Outdated Show resolved Hide resolved
bedcov.c Outdated Show resolved Hide resolved
@jmarshall
Copy link
Member Author

jmarshall commented Feb 22, 2021

Some of them seem to keep the "continue anyway" behaviour, albeit with returning an error code now.

IMHO the defining quality of the “continue anyway” messages is that they are reassuring, which these aren't.

However I hadn't tested mpileup and depth with -a and you are quite right that in this case the terminating region code prints masses of misleading incorrect output. Code lifted up as suggested, and the error message still appears after all the output hence is noticeable.

For bedcov, I was imagining later regions in the BED file might avoid the corrupted parts of the file hence be salvageable. But how would the user know which ones to trust? So I've also adjusted this code as suggested.

Thanks for the review.

@daviesrob
Copy link
Member

Thanks for the updates, will squash and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

samtools treats many errors as EOF, silently hiding problems
2 participants