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

Remove blocking ReadFrom in threadSafeBuffer #2086

Merged
merged 2 commits into from May 28, 2019

Conversation

@squaremo
Copy link
Member

commented May 23, 2019

Because ReadFrom will block on the src io.Reader it is passed, and
thereby hold the lock indefinitely, we don't want to have it as part
of the threadSafeBuffer.

Instead, don't embed the bytes.Buffer so as not to inherit ReadFrom,
and implement just the minimum needed for io.Reader and io.Writer.

Fixes #2082.

@2opremio

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

Thanks @squaremo 🤦‍♂

@2opremio

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

@squaremo For extra points, can you convert you repro gist into a test?

@squaremo

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

can you convert you repro gist into a test?

A bit fiddly but far from impossible. The main hurdle would be to not take seconds to fail (or worse, seconds to succeed). Secondary hurdle: finding a reasonable, cross-platform executable to test with.

squaremo added 2 commits May 23, 2019
Remove blocking ReadFrom in threadSafeBuffer
Because ReadFrom will block on the src io.Reader it is passed, and
thereby hold the lock indefinitely, we don't want to have it as part
of the threadSafeBuffer.

Instead, don't embed the bytes.Buffer so as not to inherit ReadFrom,
and implement just the minimum needed for io.Reader and io.Writer.

@squaremo squaremo force-pushed the issue/2082-threadsafety branch from 483ae59 to 5e1d4fa May 28, 2019

@squaremo

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

finding a reasonable, cross-platform executable to test with.

On second thought, I can't think of a reason just using git isn't adequate.

@squaremo squaremo merged commit b9a4028 into master May 28, 2019

2 of 3 checks passed

tag-filter tag-filter
Details
ci/circleci: build Your tests passed on CircleCI!
Details
helm-lint helm-lint
Details

@squaremo squaremo deleted the issue/2082-threadsafety branch May 28, 2019

@hiddeco hiddeco added this to the v1.13.0 milestone May 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.