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

Avoid data race warning in SmilesParse.cpp #2946

Merged
merged 3 commits into from
Feb 12, 2020
Merged

Conversation

skearnes
Copy link
Contributor

This PR avoids a data race identified by TSan in SmilesParse.cpp. I'm not sure it's the best solution---the race could still exist if multiple callers of {Smarts,Smiles}ToMol use different settings for debugParse---but it avoids the issue in the usual case of repeated calls to {Smarts,Smiles}ToMol with the same params.

Avoid data race in SmilesParse.cpp for SMILES and SMARTS
@bp-kelley
Copy link
Contributor

Yeah, the yysmiles_debug is certainly not thread safe. I practice though I doubt one would ever get a data race. I'm tentatively ok with this solution, the only other one I can think of is to pass the SmilesParserParams object to all the functions, but this seems like a big rewrite.

@greglandrum
Copy link
Member

Agreed: I see that this is not thread safe - in the event that everyone was ever actually running the parser in debug and non-debug modes simultaneously in separate threads - but I don't see how the proposed solution helps with any possible race condition.

If this is something we actually choose to worry about, we could add a mutex around yysmiles_debug, but I know that's going to make some folks unhappy.

@skearnes
Copy link
Contributor Author

Agreed that this PR doesn't fix the race, but it does make TSan happy in the case of not changing options :)

@greglandrum greglandrum changed the title Avoid data race in SmilesParse.cpp Avoid data race warning in SmilesParse.cpp Feb 11, 2020
@greglandrum greglandrum added this to the 2019_09_4 milestone Feb 11, 2020
@greglandrum
Copy link
Member

LGTM
@bp-kelley : please merge if you agree

@bp-kelley
Copy link
Contributor

Let’s at least add a doc note that params.debugParse Shouldn’t be used when multi threading. I can push a commit if you like.

@greglandrum
Copy link
Member

I think It’s fine to use it multithreaded, you just shouldn’t us different values on different threads

@skearnes
Copy link
Contributor Author

I can adjust the comments I added here to make this clearer...just a sec.

@skearnes
Copy link
Contributor Author

There you go; PTAL.

@greglandrum
Copy link
Member

Great. Thanks

@greglandrum greglandrum merged commit 40b9828 into rdkit:master Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants