base: Make base::CommandLine thread safe [chromium/src : main]
Nathan Hebert (Gerrit)
Nathan Hebert added 2 comments![]( "Open in Gerrit")
Nathan HebertHmm. I'm looking around Chrome at the usage and see some places where the `argv()` and `GetSwitches()` changes can cause problems. I'll see what the extent of the issues are and re-evaluate. Sorry for the noise.
Done. I added helper classes for making argv() and GetSwitches() easier to use while still being thread safe.
Hi Daniel,
Sorry for some noise earlier this week. I am adding you as an owner of base::command_line. Feel free to suggest another reviewer for the `base::` code if needed.
Best regards,
Nathan
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Hebert (Gerrit)
Nathan Hebert added 1 comment![]( "Open in Gerrit")
operator StringVector() && { return std::move(argv_); }👍, ClangTidy. I am removing the implicit conversions.
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Sorry, I'd rather not land this. It's only a partial solution at best and introduces potential contention/jank between threads. While it might prevent or reduce the crashes, it doesn't really address other issues caused by this sort of potentially racy global mutation, e.g. if one thread is setting a flag on the singleton, and another thread is trying to use the singleton at the same time, whether or not the second thread sees the flag is non-deterministic.
@g...@chromium.org I guess we should seriously consider projects to make the global singleton read-only as you mentioned a few weeks ago...
Related details
- Nathan Hebert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nathan Hebert (Gerrit)
Nathan Hebert added 1 comment![]( "Open in Gerrit")
Sorry, I'd rather not land this. It's only a partial solution at best and introduces potential contention/jank between threads. While it might prevent or reduce the crashes, it doesn't really address other issues caused by this sort of potentially racy global mutation, e.g. if one thread is setting a flag on the singleton, and another thread is trying to use the singleton at the same time, whether or not the second thread sees the flag is non-deterministic.
@g...@chromium.org I guess we should seriously consider projects to make the global singleton read-only as you mentioned a few weeks ago...
Thanks for the response. I agree that this could result in logic race conditions.
I was talking with another person in my office before lunch today, and started looking at what it would take to make `base::CommandLine::ForCurrentProcess()` return a const pointer. I can share my findings later today when I finish tracking down each of the places where the Chrome code breaks CommandLine const-ness.
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |