Fix a data race in uchime_ref and remove a dead, racy scorematrix global#637
Merged
Merged
Conversation
chimera_thread_core read the query-file progress position with fasta_get_position(query_fasta_h) while holding mutex_output, but another worker advances the same shared handle in fasta_next() (which writes file_position via fastx_file_fill_buffer) while holding mutex_input. The two accesses to file_position were serialized by different mutexes, so they raced (reported by ThreadSanitizer). Read the position into a worker-local immediately after fasta_next(), under mutex_input, and assign the shared progress counter from that local under mutex_output. This matches how search and sintax already capture progress inside the input lock. The reported progress value is unchanged.
The file-static scorematrix array was written by search16_init() but never read: the aligner uses the per-instance s->matrix. The write was therefore dead, and because search16_init() runs inside the worker threads on the chimera path (chimera_thread_init), multiple workers wrote the shared global concurrently — a latent data race on a value nobody consumes. Drop the global and its lone write. No behavioral change (s->matrix is unaffected); the dead store and the latent race both go away.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two small, independent thread-safety fixes in the alignment/chimera code,
both surfaced by a ThreadSanitizer run of the test suite.
1. Data race on the query file position in
uchime_refIn
chimera_thread_core, theuchime_refpath read the query-file progressposition with
fasta_get_position(query_fasta_h)while holdingmutex_output,but another worker advances the same shared handle in
fasta_next()— whichwrites
file_positionviafastx_file_fill_buffer()— while holdingmutex_input. The two accesses tofile_positionwere serialized by differentmutexes, so they raced.
Fix: capture the position into a worker-local immediately after
fasta_next(),under
mutex_input, and assign the sharedprogresscounter from that localunder
mutex_output— matching howsearchandsintaxalready captureprogress inside the input lock. The reported progress value is unchanged.
2. Remove the unused file-scope
scorematrixinalign_simdThe file-static
scorematrixarray was written bysearch16_init()but neverread (the aligner uses the per-instance
s->matrix). The write was dead, andbecause
search16_init()runs inside the worker threads on the chimera path,multiple workers wrote the shared global concurrently — a race on a value nobody
consumes. Dropping the global and its lone write removes both the dead store and
the race, with no behavioral change.
Testing
-O2).uchime_refruns reports no races afterthe fixes; alignment output is unchanged.