Initialize ICU when initializing Intl in order to catch data file issues early - #4984
Conversation
|
This might be okay as a temporary mitigation, but I would like to better understand the failures that are occurring. |
|
Sure. For what its worth, I also realized that I can probably check for this only in GetLocaleData and IsXLocaleAvailable, since those are the first functions that get called in the normal creation flow for Intl objects, and so if ICU is going to fail to load the data file, that's the only time it should happen. |
|
Just a thought, but can you perhaps call |
|
"u_init() will attempt to load some part of ICU's data … A successful invocation of u_init() does not, however, guarantee that all ICU data is accessible." Any clarification on that? Why would it not be a guarantee? |
|
With ICU you can totally have some of the data broken out into separate ".res" files that might sit along side the "main" .dat file. This technique is used for some of the timezone related files, in order to more easily update them and service them. |
…ues early If ICU can't load the data file, it will report back U_FILE_ACCESS_ERROR or U_MISSING_RESOURCE_ERROR on calls to ICU APIs that require the data file. When using Windows ICU, the only reason we wouldn't be able to load the data file is if we are out of memory (with bundled ICU, the data should be statically or dynamically linked, and this shouldn't be an issue). As such, to reduce noise from ICU_ASSERT, we can load the data file as soon as we initialize Intl using u_init, and if that fails with a known error code, we can treat it as OOM.
jackhorton
changed the title
|
@jefgen Updated to use u_init on initialization of Intl |
|
Thanks @jackhorton this LGTM. Just to add a few notes, for future reference: ICU is theoretically supposed to be able to function without a data file at all, and with that in mind the error code U_MISSING_RESOURCE_ERROR isn’t exactly "wrong" per-se, though it is definitely not how I would think of things. From stepping through the debugger and modifying the registers, I can confirm that if you do call the u_init() function and the data file fails to be mapped into memory then you will indeed get back U_FILE_ACCESS_ERROR -- which could be directly translated to OOM in the Chakra code. In short, I think that perhaps some sort of ‘lazy-init’ call to the u_init() function before you make any ICU API calls might be the best approach for this issue. |
| if (status == U_MEMORY_ALLOCATION_ERROR || status == U_FILE_ACCESS_ERROR || status == U_MISSING_RESOURCE_ERROR) | ||
| { | ||
| // Trace that this happens in case there are build system changes that actually cause the data file to be not found | ||
| INTL_TRACE("Could not initialize ICU - u_init returned status %S", u_errorName(status)); |
There was a problem hiding this comment.
Thanks for adding a trace here as well. It might be helpful in the future to know if the error code is File Access error or memory allocation error.
|
@dotnet-bot test OSX static_osx_osx_release please |
…der to catch data file issues early Merge pull request #4984 from jackhorton:icu/oom If ICU can't load the data file, it will report back U_FILE_ACCESS_ERROR or U_MISSING_RESOURCE_ERROR on calls to ICU APIs that require the data file. When using Windows ICU, the only reason we wouldn't be able to load the data file is if we are out of memory (with bundled ICU, the data should be statically or dynamically linked, and this shouldn't be an issue). As such, to reduce noise from ICU_ASSERT, we can load the data file as soon as we initialize Intl using u_init, and if that fails with a known error code, we can treat it as OOM.
|
FWIW LGTM too |
…Kit ICU Merge pull request #5011 from jackhorton:icu/init-windows-kit Third attempt at this after #4984 and #5001, but I have actually confirmed that this makes Node-ChakraCore's chakracore-master branch build, launch, and run Intl code successfully once more. This definitely isn't the best solution, however if we refer to https://github.com/nodejs/node-chakracore/blob/d94b22785fb6ab7dde77cb13b7e95e958e581375/src/node_i18n.cc#L568-L581, we can see that Node somewhat explicitly also does not call u_init if they call udata_setCommonData.
6 participants
If ICU can't load the data file, it will report back U_FILE_ACCESS_ERROR or U_MISSING_RESOURCE_ERROR on calls to ICU APIs that require the data file. When using Windows ICU, the only reason we wouldn't be able to load the data file is if we are out of memory (with bundled ICU, the data should be statically or dynamically linked, and this shouldn't be an issue). As such, to reduce noise from ICU_ASSERT, we can load the data file as soon as we initialize Intl using u_init, and if that fails with a known error code, we can treat it as OOM.