Fix the invariant that fd->dispatcher thread exists if fd->h is set#1154
Fix the invariant that fd->dispatcher thread exists if fd->h is set#1154jmarshall wants to merge 1 commit intosamtools:developfrom
Conversation
Remove `fd->dispatcher` test as it makes unwarranted assumptions about the pthread_t type. In fact this code assumes that fd->dispatcher is set up whenever fd->h has been set (see the unguarded pthread_join()). Fix pthread_create() error handling accordingly. (Also use the canonical name for sam_hdr_destroy().)
|
(Note that in the current develop code, if a |
|
It's been hammered on linux, but agreed there are probably poor bits of code that don't work well on other platforms. Getting the SAM threading to work with valid data and no errors is fine. The problems all come when you have a failure - malformed data, running out of memory, and so on. If I recall the line was added after a huge round of fuzzing where we died after the Nth malloc call, to test as many possible failure code paths as possible. The number of ways it could die (obviously) was huge. Things like failing to launch the thread would have been one of those that lead to deadlocks. I don't want to simply undo that check, but yes it needs to cope with the thread state being a struct. Given SAM_state is internal it could just have a status integer to indicate if it's been initialised and use that instead of a boolean comparison on a pthread_t. |
Replaces samtools#1154 It does this by the addition of a separate variable to do the boolean check on instead of fd->dispatcher itself. I deemed this easier to understand than overloading the interpretation of fd->h being set plus I'm unsure of the potential failure case in sam_hdr_fill_hrecs where fd->h has been set but we errored before creating the threads. Also improves error recovering in case of pthread creation failure, avoiding a false pthread_join later. Thanks to John Marshall for identifying that issue.
Replaces samtools#1154 See also samtools#1153 It does this by the addition of a separate variable to do the boolean check on instead of fd->dispatcher itself. I deemed this easier to understand than overloading the interpretation of fd->h being set plus I'm unsure of the potential failure case in sam_hdr_fill_hrecs where fd->h has been set but we errored before creating the threads. Also improves error recovering in case of pthread creation failure, avoiding a false pthread_join later. Thanks to John Marshall and Anders Kaplan for identifying that issue.
Replaces samtools#1154 See also samtools#1153 It does this by the addition of a separate variable to do the boolean check on instead of fd->dispatcher itself. I deemed this easier to understand than overloading the interpretation of fd->h being set plus I'm unsure of the potential failure case in sam_hdr_fill_hrecs where fd->h has been set but we errored before creating the threads. Also improves error recovering in case of pthread creation failure, avoiding a false pthread_join later. Thanks to John Marshall and Anders Kaplan for identifying that issue.
[An alternative take on the pthread
fd->dispatcherproblem identified in #1153.]Remove
fd->dispatchertest as it makes unwarranted assumptions about thepthread_ttype. In fact this code block assumes thatfd->dispatcheris set up wheneverfd->hhas been set (see the unguardedpthread_join()further down the function).Fix
pthread_create()error handling accordingly. (Also use the canonical name forsam_hdr_destroy().)This may be incomplete: it may be worth making the same change in the other nearby error handling; more significantly, the relationship between the thread pool in
fd->qand the dispatcher thread is unclear, so resettingfd->hmay not be quite right in some cases… maybe some of thefd->qhandling needs to change too.