Skip to content

zstd: pooledZipWriter should return Writers to the same pool#426

Merged
klauspost merged 1 commit intoklauspost:masterfrom
mostynb:zstd_writer_pool_fix
Aug 30, 2021
Merged

zstd: pooledZipWriter should return Writers to the same pool#426
klauspost merged 1 commit intoklauspost:masterfrom
mostynb:zstd_writer_pool_fix

Conversation

@mostynb
Copy link
Copy Markdown
Contributor

@mostynb mostynb commented Aug 30, 2021

This code was previously Get'ing objects from one pool and later Put'ing them to a different pool, so they were never reused.

@mostynb
zstd: pooledZipWriter should return Writers to the same pool
085af16 
This code was previously Get'ing objects from one pool and later
Put'ing them to a different pool, so they were never reused.
@mostynb
Copy link
Copy Markdown
Contributor Author

mostynb commented Aug 30, 2021 •
edited
Loading

Also, pooledZipReader seems to be unused because it leaks goroutines- would you be interested in merging something like https://github.com/mostynb/zstdpool-syncpool/blob/main/decoderpool.go (as an un-exported type)?

@klauspost
Copy link
Copy Markdown
Owner

klauspost commented Aug 30, 2021

@mostynb Nice! Thanks!

I don't want to rely on finalizers. I want to fix it properly, ie have a streaming option that only uses the main goroutine.

@mostynb
Copy link
Copy Markdown
Contributor Author

mostynb commented Aug 30, 2021

I don't want to rely on finalizers. I want to fix it properly, ie have a streaming option that only uses the main goroutine.

I agree that would be preferable. Is it worth commenting out pooledZipReader in the meantime, since it's unused?

@klauspost
Copy link
Copy Markdown
Owner

klauspost commented Aug 30, 2021

@mostynb It is fine to leave. Thanks for the contribution! I will include it in the release I will do shortly.

@klauspost klauspost merged commit e632408 into klauspost:master Aug 30, 2021
@mostynb mostynb deleted the zstd_writer_pool_fix branch August 30, 2021 08:58
@hyunuk hyunuk mentioned this pull request Nov 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mostynb @klauspost