-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Replace usage of std::ctime, which is not safe in multithread contexts #3228
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
Replace usage of std::ctime, which is not safe in multithread contexts #3228
Conversation
…ed contexts - std::ctime returns a pointer to a string that "may be shared between std::asctime and std::ctime, and may be overwritten on each invocation of any of those functions.". - https://en.cppreference.com/w/cpp/chrono/c/ctime - Replaced with call to strftime to generate the same string representation (using the format string: %c) - Leveraged localtime_r (which is thread-safe) to convert time_t to struct tm, as required by strftime.
e946f9c
to
9cbd699
Compare
The sonarcloud reported issues recommend replacing the use of
|
9cbd699
to
4e9d63e
Compare
- Leverage std::size to determine buffer size at compile time. - Simplified 'TimeMon::evaluate' implementation as it was using strftime to get the month, convert the string to int, and then decrement it by one to make it zero based. This same value is already available in the 'struct tm' previously generated with the call to localtime_r (and where the month is already zero-based)
4e9d63e
to
23a341e
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks!
I saw. I updated SonarCloud settings, now it checks the code with C++17 standards. We will see the result... |
That's great! I think that should reduce noise (that is, we won't need to dismiss issues because they don't apply, at least yet) Thx! |
what
Replace usage of
std::ctime
in the string util ascTime, which is not safe in multithreaded contexts.ascTime
is used byTransaction::toJSON
, which is used to log transactions.why
From IBM's ctime documentation:
This was reported by
sonarcloud
while working on PR #3222:changes
std::ctime
with a call tostrftime
, where the output buffer is provided as an argument and is thread-safe.strftime
to leveragestd::size
to reserve the space needed by the format string.TimeMon::evaluate
which was unnecessarily generating a string representation of the month to then parse it as an integer value, when the value is already available in thestruct tm timeinfo
with the call tolocaltime_r
.notes
Checked that the library doesn't currently include any call to
localtime
(which is not thread-safe either) and useslocaltime_r
instead.