Skip to content

bgzf_read_block added error return#1529

Merged
whitwham merged 3 commits intosamtools:developfrom
bir3:tabix-on-truncated-file-1528
Dec 15, 2022
Merged

bgzf_read_block added error return#1529
whitwham merged 3 commits intosamtools:developfrom
bir3:tabix-on-truncated-file-1528

Conversation

@bir3
Copy link
Contributor

@bir3 bir3 commented Nov 23, 2022

No description provided.

@bir3
Copy link
Contributor Author

bir3 commented Dec 10, 2022

fixed memory leak

@whitwham
Copy link
Member

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

@bir3
Copy link
Contributor Author

bir3 commented Dec 13, 2022

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:

  • One view is to say that it should yield identical results, e.g. valid blocks are returned until we hit a truncated block.
  • One can also say that in the presence of a truncated file it is valid to return error early and not return some valid blocks.

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
Copy link
Member

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
Copy link
Contributor Author

bir3 commented Dec 14, 2022

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

@whitwham whitwham changed the title draft fix bgzf_read_block added error return Dec 15, 2022
@whitwham whitwham merged commit d7737aa into samtools:develop Dec 15, 2022
@whitwham
Copy link
Member

Thank you for your fix.

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.

2 participants

Comments