ICU initialization for properly work in MT environment [CORE2642]

[firebird-automations]

Submitted by: @ibprovider

Attachments:
module_initialization.cpp

At durring a multithreaded stress tests I got the problems with ICU. Later I found in extern\icu\readme.html the next informations

-
Using ICU in a Multithreaded Environment

Upon the first usage of most ICU APIs, the global mutex will get initialized properly, but you can use the u_init() function from uclean.h to ensure that it is initialized properly. Without calling this function from a single thread, the data caches inside ICU may get initialized more than once from multiple threads, which may cause memory leaks and other problems. There is no harm in calling u_init() in a single threaded application.

-
I added in to icuuc30.dll the DllMain function (see attach) and made two pass of my tests - all work fine without any problems.

(But I use the umtx_init instead u_init, because u_init can return the error at durring ICU compilations).

Also, I added the call of umtx_cleanup for releasing the resources of ICU critical sections. With u_cleanup I got the problem in x64 build.

Could you add this (or similar) code to your ICU "common" project?

Commits: 44c409a

[firebird-automations]

Commented by: @ibprovider

DllMain for icuuc30.dll

[firebird-automations]

Modified by: @ibprovider

Attachment: module_initialization.cpp [ 11501 ]

[firebird-automations]

Modified by: @asfernandes

assignee: Adriano dos Santos Fernandes [ asfernandes ]

[firebird-automations]

Commented by: @asfernandes

Calling umtx_init(NULL) is related to implementation details not documented. u_init was been marked as DRAFT in 3.0, but apparently was been later marked as stable since 2.6. What compilation problem did you had?

But u_cleanup, IMO, may cause more harm than good. Suppose one application loads fbembed and ICU, then unload fbembed. u_cleanup does not do referencing counting, so it will clean ICU used by the application.

For the DllMain, that is how really they should do it. But I would not like for us to fix all they problems, this one, for example, has a very low surface: fbembed being loaded and unloaded will cause leaks.

I'll verify more recent versions and report a bug for them if appropriate.

[firebird-automations]

Commented by: @ibprovider

>Calling umtx_init(NULL) is related to implementation details not documented.

documentation - for external users. "DllMain" - this is "internal" user :)

> u_init was been marked as DRAFT in 3.0, but apparently was been later marked as stable since 2.6. What compilation problem did you had?

u_init returns the status code. When I tried to detect and process fail-status-code (and return the FALSE from DllMain) genrb.exe was fail

---
IBProvider+fbembed can to use ICU library together

So, ICU DLL should independently initialize and deinitialize the own global internal resources (for example, in DllMain code)

This is general rule for all normal DLLs.

----
As I understand (but not sure) u_cleanup produces the next problem:
- icuin30.dll registers the callback function in icuuc30.dll
- icuin30.dll unloads from memory
- icuuc30.dll calls the function by wrong pointer

Then I decide do not use the u_cleanup, and use the umtx_cleanup only

----
Thank you.

[firebird-automations]

Modified by: @dyemanov

Fix Version: 2.5 RC1 [ 10362 ]

[firebird-automations]

Commented by: @asfernandes

Fixed only the initialization problem, and not the cleanup.

I'd hate to change ICU sources, and it will not work for Linux distributions that uses system ICU anyway.

[firebird-automations]

Modified by: @asfernandes

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

[firebird-automations]

Modified by: @pcisar

status: Resolved [ 5 ] => Closed [ 6 ]

[firebird-automations]

Modified by: @pavel-zotov

QA Status: No test

[firebird-automations]

Modified by: @pavel-zotov

status: Closed [ 6 ] => Closed [ 6 ]

QA Status: No test => Cannot be tested

[firebird-automations]

Commented by: @ibprovider

http://firebirder.ru/problemy-s-icu-podxod-2

[firebird-automations]

Commented by: @hvlad

I found this ticket when fixing some memory\resource leak in v3.
As you know, v3 have plugin-based arcitecture and its dispatcher unloaded unsed modules after some period of inactivity.
But i found that two ICU libraries (icuuc52.dll and icudt52.dll) and data file (icudt52l.dat) was left in process address space
after engine (and trace, and intl) plugins are unloaded.
Also there is small memory leak related with not released ImplementConversionICU instance (convIcu variable).
When i fixed memory leak i found that icudt52l.dat still left at process address space.
Worse, when ICU loaded again, it creates new instance of file mapping.
Then i add call of u_cleanup() right before delete of convIcu and it seems it finally fixed an issue.
But then i found this ticket and see the reason why it was not done before ;)

Things are changed since v2.5. I think we should reconsider old decision and found a way to properly release ICU
libraries when it is safe. It is definitely safe in case of full server and is questionable in case of embedded application.
I could think on following ways to deal with it:
- introduce new setting in firebird.conf to enable\disable unloading of ICU, or
- introduce new API call at Master interface (or another one) to enable\disable unloading of ICU.

Opinions ?

PS i found strange that engine load icuuс* module twice - one time as ImplementConversionICU and
second time at UnicodeUtil::loadICU as ICU::ucModule

[firebird-automations]

Commented by: @ibprovider

IBProvider do not call u_init and u_cleanup.

If i remember correctly - these function not have counter of calls and not thread safe. Please correct me, if I wrong.

Instead, I use ICU with added DLLMain, which call u_init and u_cleanup.

No memory leaks and other problems.

IBProvider supports the work of icu3 and icu52.

[firebird-automations]

Commented by: @hvlad

We can't change ICU's DllMain as we should work with any user-supplied ICU library.
But it is good to know that u_init\u_cleanup really works ;)

[firebird-automations]

Commented by: @ibprovider

u_cleanup from ICU3 contains the BUG, as I explain in my blog record.

[firebird-automations]

Commented by: @dyemanov

Runtime setting via IMaster sounds good for me.

[firebird-automations]

Commented by: @asfernandes

I think it's not good to expose implementation detail (even if we depends too much of it as in this case) in the API.

[firebird-automations]

Commented by: @hvlad

Adriano,

nobody likes it, sure. But... do you have any other suggestion ?

[firebird-automations]

Commented by: fbbt (fbbt)

>> But i found that two ICU libraries (icuuc52.dll and icudt52.dll) and data file (icudt52l.dat) was left in process address space
>> after engine (and trace, and intl) plugins are unloaded.

Sorry, but someone found it first :(

CORE5299: embedded: after fb_shutdown some files stay locked
icudt52l.dat
ib_util.dll
icudt52.dll
icuuc52.dll
msvcr100.dll

[firebird-automations]

Commented by: @hvlad

Adriano,

could you comment on double loading of conversion library ?
Is is necessary or we could (should) avoid it ?

[firebird-automations]

Commented by: @asfernandes

Initially, ICU was always loaded statically.

Later, part of ICU was loaded statically and partially dynamic, as collations keys depends on ICU version. The code you see reflects this stage.

Later then, Alex changed to load all functions from ICU dinamically, but the above thing was not changed.

What is important here, we support loading of more than one version for collations, while some things requires at least a (any) version.

[dmitry-lipetsk]

Just for additional information about ICU initialization/deinitialization.

ICUIN (v63) module, which is used in FB4 and provides functions for working with time zones, registers cleanup functions in ICUUC module.

So, if you want to call u_cleanup function from ICUUC, it need to do this before unload ICUIN module.

Otherwise you will get AV.