Bug: Possible race condition in destination
- Dominant language
- Go
- Stars
- 610
- Forks
- 63
- Avg merge
- 12h 28m
- Merged PRs (30d)
- 57
Description
### Bug description
We have a _possible_ race condition in `DestinationNode` and `DestinationAckerNode`. Imagine `DestinationNode` successfully [writes a record](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination.go#L106) to the destination connector. The destination plugin fails to process the record (for whatever reason) and sends back a negative acknowledgment to Conduit. In the meantime `DestinationNode` [sends](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination.go#L120) the record to `DestinationAckerNode` which [receives the nack](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination_acker.go#L127), tries to handle it but [experiences an error](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination_acker.go#L155) (e.g. DLQ is not configured). This causes `DestinationAckerNode` to stop fetching new acks, return an error that [kills the tomb](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/lifecycle.go#L515) and cancels the context shared by all pipeline nodes, including `DestinationNode`. Meanwhile `DestinationNode` might have received a second record and has already [written it](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination.go#L106) to the destination connector. While trying to [send](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination.go#L120) the record to `DestinationAckerNode` it notices that the context is cancelled so it [nacks](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination.go#L122) the message and tries to signal a [stop](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination.go#L70) to the destination. The problem now is that the destination will try to send an acknowledgment for the second record back to Conduit but won't be able to since `DestinationAckerNode` is not listening to acks anymore, which could result in the stream staying open indefinitely.
You might think that `DestinationNode` could fetch the ack itself since it knows that a record was written to the connector. The problem is that it can't know for sure that `DestinationAckerNode` is not listening to acks anymore, it might be that the context was closed because of another error, so `DestinationNode` could receive an ack for another message, meant to be received by `DestinationAckerNode`.
You might also say that `DestinationAckerNode` should just keep fetching acks until the stream is closed. In fact `DestinationAckerNode` previoiusly had this logic. It spawned a [cleanup goroutine](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination_acker.go#L197-L203) that was trying to fetch acks until it experienced an error (i.e. the stream was closed) but that was resulting in a deadlock between this goroutine and `DestinationNode` that was trying to [tear down](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/pipeline/stream/destination.go#L85) the connector and was [waiting](https://github.com/ConduitIO/conduit/blob/929fe129bbc9ea29c98b7d81b5f7eb3f39127a1a/pkg/connector/destination.go#L140-L141) for existing calls to finish first. Now the cleanup goroutine is only [fetching acks](https://github.com/ConduitIO/conduit/blob/d8217d4e03dd4697a68e3705e868c567f842a0bf/pkg/pipeline/stream/destination_acker.go#L198) for records it knows about.
### Steps to reproduce
See above.
### Version
v0.5.0
Contributor guide
Assessment
This issue has not been assessed yet.