Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uploader: propagate errors in compress #137

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

teqwve
Copy link
Contributor

@teqwve teqwve commented Jan 30, 2024

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
Copy link
Contributor Author

teqwve commented Feb 5, 2024

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 #138:

  • ClickHouse returns 200 ok without reading the whole request, before carbon clickhouse has sent the whole file
  • go http client closes the read end of body pipe
  • io.Copy in compress goroutine fails with ErrClosedPipe and exits without propagating the error upstream (here)
  • parseAndFilter in worker goroutine is blocked forever on writing to a pipe nobody is going to read from (here)

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

@teqwve teqwve force-pushed the propagate-error-in-compress branch from 1553947 to 5a12163 Compare February 5, 2024 09:57
@teqwve teqwve force-pushed the propagate-error-in-compress branch from 5a12163 to db71271 Compare February 5, 2024 10:49
@msaf1980 msaf1980 merged commit 898a7d7 into go-graphite:master Feb 7, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants