Fix code writing to the database to work in any locale as well. #241
Conversation
|
This thread provides some context for why we haven't made such change in The current code is predictably and reliably broken when locale is set to For the right though inelegant way of addressing this problem look at the Thanks, Aleksander On Sun, Mar 30, 2014 at 1:08 PM, VZ notifications@github.com wrote:
|
|
Do you have any example of platforms where The Google hack is horrible and is simply due to the well-known aversion of that company to using C++ as anything other than C-with-classes-and-some-other-minor-additions. We could still use it, at the very least the existence of |
|
On Sun, Mar 30, 2014 at 2:11 PM, VZ notifications@github.com wrote:
http://www.opensource.apple.com/source/libstdcxx/libstdcxx-60/include/c++/4.2.1/bits/c++locale.h I don't have much experience reading c++ standard library source, so it's Thanks, Aleksander |
|
On Sun, 30 Mar 2014 20:49:54 -0700 Pawel Aleksander Fedorynski notifications@github.com wrote: PAF> On Sun, Mar 30, 2014 at 2:11 PM, VZ notifications@github.com wrote: Sorry, I stand corrected, libstdc++ under OS X does indeed have this PAF> I don't have much experience reading c++ standard library source, so it's Yes, and here is a quick and dirty test https://gist.github.com/vadz/9890860 It's somewhat reassuring that at least it is fixed in libc++: but as long as we support using libstdc++ under OS X, we need to deal with Thanks, |
|
New version using explicit decimal comma hack. |
|
OK good to see that the test confirms it. I think github lost the email I typed last night so sorry for duplication if it reappears: On a closer look, things seem even worse: __convert_from_v() calls setlocale() not to switch to whatever was imbued, but to always switch to "C". [And the decimal point '.' is replaced with ',' (if required) later in _M_insert_float().] So the problem is not with calling imbue(): it seems that any calls that pass a floating point number to ostream::operator<< are dangerous in multi-threaded programs when the global locale is not "C"!? And this is not just OSX with libstdc++, the code looks similar in Android NDK, OpenBSD, FreeBSD. Could be that on at least some of these platforms this is obsolete code (like libstdc++ is being obsoleted by libc++ on OSX). On Linux this has been fixed long ago by replacing setlocale() with uselocale() in __convert_from_v(). So we are probably not in a happy place because SOCI is using ostream::operator<< probably quite a lot. But in such cases it's good to fix things one place at a time and from that perspective your change is an excellent step. As for the code itself, I looked at it and it looks good. This kind of code tends to be very tricky so it would be good to get a second pair of eyes to review cstring_to_double() and double_to_cstring(). My only complaint is that it's probably better to avoid strdup() which is a non-standard extension. Also, it would be good to have some tests for these functions. Alternatively, if licenses permit, we could just copy the protobuf code I linked, which is probably thoroughly reviewed and tested. Thanks, Aleksander |
|
The existing mysql test proved to be sufficient for them, it already tests all possible cases (and did discover a bug, with not failing to parse the numbers using decimal comma in my first version).
Reusing protobuf code: it has dependencies on other Google stuff, it's probably not going to be that easy to extract. And we don't need full |
Some implementations of C++ streams (notably libstdc++ under OS X) are buggy and change the global locale to implement support for imbue(), which is not thread-safe, and so could introduce subtle bugs in multi-thread programs. Replace the old correct but not working in practice code with a new ugly version only supporting decimal comma and period which is not totally correct in theory but should work fine in practice (and shouldn't make things worse even for the hypothetical locales using some other decimal separator anyhow).
Similarly to the issue SOCI#238 but in the other direction, we must use locale-independent functions to ensure that we send the strings in correct format to the database even if the current locale doesn't use period as decimal separator. This fixes errors like invalid input syntax for type double precision: "3,1415926500000002086" when executing the tests in e.g. French locale.
Propagate the changes done to postgresql backend in sqlite3 and mysql backends: use locale-independent functions for converting between the (floating point) numbers and strings as the database itself always expects, and provides, the strings in "C" locale.
This makes it easier to check that SOCI works correctly when used in a program that changes the global locale as it can now be tested just by running tests after setting LC_ALL to some non-default value.
|
New version: just avoid |
|
I completely forgot about this potential of troubles in multi-threaded environment, and the old thread too. Thanks @pfedor for digging it out. @vadz suggested to apply this (initially #238) fix to SOCI 3.2.3 as well. I agreed it's a good idea. So, this PR #241 sound and ready to be backported to 3.2.3? |
|
I think it's a very good change and big thanks to VZ for working on it. A
Mateusz: can you review cstring_to_double() and double_to_cstring() just to Thanks Aleksander On Sat, Apr 5, 2014 at 12:23 PM, Mateusz Łoskot notifications@github.comwrote:
|
|
On Sat, 05 Apr 2014 12:58:56 -0700 Pawel Aleksander Fedorynski notifications@github.com wrote: PAF> I think it's a very good change and big thanks to VZ for working on it. A You're right, I think we should. The explanation for why I didn't do it is PAF> 2. Seems to me cstring_to_double() does not consider it an error if there's Again, this just preserves the old code behaviour. I don't think it's PAF> 3. Is cstring_to_double() being tested for cases when input contains the This is a can of worms, too. Some implementations (notably MSVC CRT) don't PAF> 4. I'm not sure it's such a good idea to use the systems locale for the Unfortunately there is no portable way to set locale in C. We could try Regards, |
| #define SOCI_PRIVATE_SOCI_DTOCSTR_H_INCLUDED | ||
|
|
||
| #include "soci/error.h" | ||
|
|
There was a problem hiding this comment.
I would add #include "soci/soci-platform.h" to provide snprintf
|
On 5 April 2014 21:58, Pawel Aleksander Fedorynski <notifications@github.com wrote:
Shall we add killer test for those two functions, something based on data
I agree. I have been testing these two functions myself with setlocale and this idea seems working well.
Both functions look solid, I'd just add ERANGE check (see my inline code Thanks for great work! |
|
BTW, a side note, I noticed perhaps related issue in MySQL utility The |
|
I'd say it's exactly right, |
|
I'd like to return to this: is it ok to merge in the current state, possibly with the improvements addressing the issues mentioned (e.g. IMHO this change at the very least doesn't make things worse, so I'd like to commit it, even if it's not ideal yet. Any objections? |
|
I would prefer for the ERANGE to be present, but I'm fine with submitting as is and adding it later. Mateusz makes the final call. BTW here's the reason why the MySQL backend checks explicitly for infinity and NaN: http://sourceforge.net/p/soci/mailman/soci-users/thread/CAE7Ac0ADBFO-qv6dshnO5RDv1syKT-qmrgFyf6Z-Z1ffCh5hmQ%40mail.gmail.com/ Thanks, Aleksander |
|
No objections. I agree with @pfedor the ERANGE check would be good to have though. Once merged, I will see if I can incorporate it into the 3.x line. p.s. Please, don't rely on me as the final caller and let's keep things democratic ;) |
|
I've finally pushed it as is because I am not sure I'm going to have time to work further on this (I have more important, IMHO, changes to SOCI in the pipeline as well) and the rest is really vastly less critical than the fact that SOCI plain out doesn't work right now in the locales using decimal comma. For the record, ideally the following would need to be done:
|
This is similar to #238 but completes it by