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

Thread-safety concern related to interpolation parsing #715

Closed
odelalleau opened this issue May 14, 2021 · 4 comments · Fixed by #716
Closed

Thread-safety concern related to interpolation parsing #715

odelalleau opened this issue May 14, 2021 · 4 comments · Fixed by #716
Assignees
Labels
bug Something isn't working In progress
Milestone

Comments

@odelalleau
Copy link
Collaborator

Describe the bug

It occurred to me that we may want to ensure that OmegaConf is thread-safe (at least on simultaneous reads). But the cache mechanism I introduced in #445 to speed-up the parsing of interpolations is not thread-safe.

Expected behavior

Parsing interpolations in parallel from multiple threads should not fail because of the cache mentioned above.

@odelalleau odelalleau added the bug Something isn't working label May 14, 2021
@odelalleau odelalleau self-assigned this May 14, 2021
@odelalleau odelalleau added this to the OmegaConf 2.1 milestone May 14, 2021
odelalleau added a commit to odelalleau/omegaconf that referenced this issue May 14, 2021
odelalleau added a commit to odelalleau/omegaconf that referenced this issue May 14, 2021
odelalleau added a commit to odelalleau/omegaconf that referenced this issue May 14, 2021
@omry
Copy link
Owner

omry commented May 14, 2021

Thread safety is not a current goal.
We have several cache mechanisms besides this one that are all potentially unsafe. So far this has not came up in user reports.

@odelalleau
Copy link
Collaborator Author

odelalleau commented May 14, 2021

Yeah I assumed it wasn't a top priority, but I also thought it wouldn't hurt to address this specific one.

For what it's worth I ended up thinking about it because I realized my own project was multi-threaded and might run into problems because of this specific issue (although it's unlikely to happen because my reads are infrequent enough). The tricky thing is that even if it happens, I may not notice it because it might not crash (one potential side effect would be reading an incorrect config value, but in my specific use case that would essentially "just" give incorrect results).

I won't push for it though if you feel like it's not worth it -- I'll probably duplicate my config instead in my project anyway just to be 100% safe :)

@omry
Copy link
Owner

omry commented May 14, 2021

We can add it. But it will be a much longer journey than this.
I will take a look next week.

@odelalleau
Copy link
Collaborator Author

We can add it. But it will be a much longer journey than this.

General thread-safety would definitely be quite challenging. Thread-safety on concurrent reads may be a more realistic target.

odelalleau added a commit that referenced this issue May 18, 2021
Ensure the grammar cache is thread-safe

Fixes #715
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working In progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants