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

Keep Channel#close thread-safety #8249

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@firejox
Copy link
Contributor

firejox commented Sep 29, 2019

If one thread is closing the channel and the other thread is sending data to channel, then both
threads may access the content simultaneously. It will be a bug under multi-thread environment.

@r00ster91

This comment has been minimized.

Copy link
Contributor

r00ster91 commented Sep 29, 2019

Shouldn't the multi-threading flag preview_mt be used here?

@firejox

This comment has been minimized.

Copy link
Contributor Author

firejox commented Sep 29, 2019

@r00ster91 The spinlock has internally use the preview_mt flag. The lock and unlock operations will be no-op if the program is compiled without the flag. Hence, I think there is no need to use preview_mt here.

@bcardiff bcardiff requested a review from waj Sep 29, 2019
@firejox firejox force-pushed the firejox:channel-closed-thread-safety branch from 9758210 to a96d6bd Oct 3, 2019
@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Oct 3, 2019

@firejox what is the use case that makes you deal with closed channels?

If possible keep the history of this PR with some experiments (or even specs) that is pushing you to iterate on the solution.

Thanks!

@firejox

This comment has been minimized.

Copy link
Contributor Author

firejox commented Oct 3, 2019

@bcardiff Sorry, I just trace the channel.cr and find there is no lock protection on Channel#close. I will rollback the change because the new change is less related to this race bug.

@firejox firejox force-pushed the firejox:channel-closed-thread-safety branch from a96d6bd to e0034ff Oct 3, 2019
@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Oct 3, 2019

I thought you've found further issues and I was intrigued by a code to stress it.

@firejox

This comment has been minimized.

Copy link
Contributor Author

firejox commented Oct 3, 2019

The new change is for the situation that some channel is closed before the Channel.select cleaning up the context. And this is another issue.

@firejox firejox force-pushed the firejox:channel-closed-thread-safety branch from e0034ff to e8adef1 Oct 8, 2019
@waj
waj approved these changes Oct 9, 2019
@waj waj added this to the 0.32.0 milestone Oct 9, 2019
@waj waj merged commit 37ca414 into crystal-lang:master Oct 9, 2019
5 checks passed
5 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.