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

TestName: Make 'name' field volatile #1157

Closed
wants to merge 1 commit into from

Conversation

rschmitt
Copy link
Contributor

@rschmitt rschmitt commented Jun 4, 2015

This ensures that the name is published across threads correctly--for
instance, if a parallelized runner is used.

This ensures that the name is published across threads correctly--for
instance, if a parallelized runner is used.
@kcooney kcooney closed this in eedd1f6 Jun 5, 2015
@kcooney
Copy link
Member

kcooney commented Jun 5, 2015

Merged. Thanks!

I think in practice, we were probably fine, since it would be hard for a thread that did not update that field to get a reference to the rule. But making it volatile is safer and there's almost no overhead on most processors.

@rschmitt
Copy link
Contributor Author

rschmitt commented Jun 5, 2015

Suppose you have a Timeout downstream of TestName. The name field will be set, and then Timeout will spawn a separate thread in which the actual test will run. Currently this should be safe because it creates a new Thread object for every test invocation, but if this implementation detail ever changed (e.g. Timeout reuses threads in a thread pool) undefined behavior will result.

@kcooney
Copy link
Member

kcooney commented Jun 5, 2015

Ah, I see. I could easily imagine custom Timeout implementations that use a thread pool

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