Issue Details (XML | Word | Printable)

Key: CORE-2642
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Adriano dos Santos Fernandes
Reporter: Kovalenko Dmitry
Votes: 0
Watchers: 3
Operations

If you were logged in you would be able to see more operations.
Firebird Core

ICU initialization for properly work in MT environment

Created: 26/Sep/09 11:52 PM   Updated: 20/Apr/18 10:46 AM
Component/s: Charsets/Collation
Affects Version/s: 2.1.0, 2.1.1, 2.1.2, 2.5 Beta 1, 2.5 Beta 2, 2.1.3
Fix Version/s: 2.5 RC1

File Attachments: 1. Text File module_initialization.cpp (1 kB)

Environment: Multithread stress tests

QA Status: Cannot be tested


 Description  « Hide
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?

 All   Comments   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Kovalenko Dmitry added a comment - 26/Sep/09 11:56 PM
DllMain for icuuc30.dll

Adriano dos Santos Fernandes added a comment - 27/Sep/09 01:43 AM
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.

Kovalenko Dmitry added a comment - 27/Sep/09 02:21 AM
>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.

Adriano dos Santos Fernandes added a comment - 05/Oct/09 01:26 AM
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.


Vlad Khorsun added a comment - 19/Apr/18 10:39 AM - edited
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

Kovalenko Dmitry added a comment - 19/Apr/18 11:24 AM
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.

Vlad Khorsun added a comment - 19/Apr/18 11:34 AM
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 ;)

Kovalenko Dmitry added a comment - 19/Apr/18 11:56 AM
u_cleanup from ICU3 contains the BUG, as I explain in my blog record.

Dmitry Yemanov added a comment - 19/Apr/18 12:37 PM
Runtime setting via IMaster sounds good for me.

Adriano dos Santos Fernandes added a comment - 19/Apr/18 03:15 PM
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.

Vlad Khorsun added a comment - 19/Apr/18 03:20 PM
Adriano,

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

fbbt added a comment - 20/Apr/18 03:44 AM
>> 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 :(

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


Vlad Khorsun added a comment - 20/Apr/18 08:53 AM
Adriano,

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

Adriano dos Santos Fernandes added a comment - 20/Apr/18 10:46 AM
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.