asynchronous close(batchPipeline) in kafka/main.go can lead to panic #706
Labels
bug
Something isn't working
Comments
ibukanov
added a commit
that referenced
this issue
May 21, 2024
The current code closes the batchPipeline channel when it receive transient errors to restart the readers. But this does not close Kafka readers so they continue to read. When this happens, a message will be written to a closed channel triggering panic in processMessagesIntoBatchPipeline. As that does not use recover, this will crash the whole process. This change refactors error handling to ensure that the errors are properly propagated to the caller while also supporting context cancellation which allowed to provide extended test coverage. The reader are always closed on errors. CLOSE #706
ibukanov
added a commit
that referenced
this issue
May 22, 2024
The current code closes the batchPipeline channel when it receive transient errors to restart the readers. But this does not close Kafka readers so they continue to read. When this happens, a message will be written to a closed channel triggering panic in processMessagesIntoBatchPipeline. As that does not use recover, this will crash the whole process. This change refactors error handling to ensure that the errors are properly propagated to the caller while also supporting context cancellation which allowed to provide extended test coverage. Kafka readers/writers are always closed on errors. This change still uses an explicit exit on errors and lets the container runtime to restart the process. CLOSE #706
ibukanov
added a commit
that referenced
this issue
May 23, 2024
The current code closes the batchPipeline channel when it receive transient errors to restart the readers. But this does not close Kafka readers so they continue to read. When this happens, a message will be written to a closed channel triggering panic in processMessagesIntoBatchPipeline. As that does not use recover, this will crash the whole process. This change refactors error handling to ensure that the errors are properly propagated to the caller while also supporting context cancellation which allowed to provide extended test coverage. Kafka readers/writers are always closed on errors. This change still uses an explicit exit on errors and lets the container runtime to restart the process. CLOSE #706
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
StartConsumers() from kafka/main.go calls panic(batchPipeline) when readAndCommitBatchPipelineResults() returns an error. But this will trigger panic and terminate the process if processMessagesIntoBatchPipeline() posts a new message to batchPipeline channel.
In general the logic of handling transient errors is broken. processMessagesIntoBatchPipeline() can only terminate by panic and that kills the process. If this is intended bbehavior, then the code to call StartConsumers() as a recovery should be removed.
The text was updated successfully, but these errors were encountered: