ConduitIO / ConduitIO/conduit

Bug: Possible race condition in destination

Open
#859 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.