go-graphite / carbon-clickhouse

Graphite metrics receiver with ClickHouse as storage
MIT License
187 stars 47 forks source link

uploader: propagate errors in compress #137

Closed teqwve closed 7 months ago

teqwve commented 7 months ago

Hi,

we've noticed that compress goroutine does not properly handle corner cases: it does not close upstream and it does not propagate an error in case it occurs. Because of that with compression enabled uploader behaved differently than with compression disabled (when there intermediary pipes). This PR adds a proper handling.

teqwve commented 7 months ago

Maybe I'll explain why I believe this fix is important - it may cause strange bugs, in our case it caused a worker goroutine to be blocked in a file upload. Blocked goroutines weren't releasing files (so files were never returned to the pool and re-uploaded) and once number of blocked goroutines reached configured number of upload 'threads' the upload was blocked completely.

In this case it was triggered by similar behavior as described in https://github.com/go-graphite/carbon-clickhouse/pull/138:

it wasn't happening with compression disabled, so I think that we should fix error handling in compression (in a similar way it's handled in compress goroutine example in go sources, https://github.com/golang/go/blob/201a2e9c2f82dd2c57c8e79bbe2c028d7c13b8ea/src/compress/gzip/example_test.go#L169).

I may also add that this seem to be a quite common issue, e.g. an example I've found in gist have it, https://gist.github.com/tomcatzh/cf8040820962e0f8c04700eb3b2f26be