Initialization: set Boost path locale in main thread #5877
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
dexX7
commented
Mar 11, 2015
dexX7
commented
- Boost path seems to use variables which are lazy initialized and not thread safe, and in particular this results in leaks and unexpected behavior, if they are not explicitly initialized by the main thread, because resources may no longer be accessible, if the related thread isn't either. This is basically the underlying issue I'm trying to address, and related issues affected by this are [Qt] Do you want to rebuild the block database now? No -> crash #3136 and libdb_cxx-4.8 leaks? #5380.
- path::imbue(std::locale()) appears to initialize those.
- std::locale() constructs a copy of the current global locale. The global locale is the locale set by a previous call to std::locale::global, or std::locale::classic ("C"), if std::locale::global has not been called. std::locale("") however uses the default environment value.
- Setting locale::global(loc) also sets the C global locale, as if the C library function setlocale was called with LC_ALL, which appears to be more robust, while setenv may not be available in any case.
- locale("") throw exceptions, if invalid environment locales are used, but locale() does only so, if locale::global was set with an invalid locale before, which should not be the case in Bitcoin Core, so I'd assume the old L732 never throws at all.
- I tested it with several build configurations, but I'm not entirely sure, if this is 100 % side effect free.
The path locale is lazy initialized and to avoid deinitialization errors in multithreading environments, it is set explicitly by the main thread.
Member
There was a problem hiding this comment.
Are you sure this will still work for boost filesystem v3? I'd assume this #ifdef construction was added for a reason.
Contributor
Author
There was a problem hiding this comment.
Yes, it's actually std::locale("") what raises the exception, which is called from within path::codecvt().
Member
Contributor
|
Tested ACK, #3136 is fixed with this PR :), great! |
laanwj
added a commit
that referenced
this pull request
Mar 24, 2015
317e66c Initialization: set Boost path locale in main thread (dexX7)
Member
|
Cherry-picked to 0.10 as c9e022b |
reddink
pushed a commit
to reddcoin-project/reddcoin-3.10
that referenced
this pull request
May 27, 2020
The path locale is lazy initialized and to avoid deinitialization errors in multithreading environments, it is set explicitly by the main thread. Conflicts: src/util.cpp Rebased-From: 317e66c Github-Pull: bitcoin#5877 (cherry picked from commit c9e022b)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.