Skip to content
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

ICU initialization for properly work in MT environment [CORE2642] #3049

Closed
firebird-automations opened this issue Sep 27, 2009 · 23 comments
Closed

Comments

@firebird-automations
Copy link
Collaborator

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
Copy link
Collaborator Author

Commented by: @ibprovider

DllMain for icuuc30.dll

@firebird-automations
Copy link
Collaborator Author

Modified by: @ibprovider

Attachment: module_initialization.cpp [ 11501 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @asfernandes

assignee: Adriano dos Santos Fernandes [ asfernandes ]

@firebird-automations
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Modified by: @dyemanov

Fix Version: 2.5 RC1 [ 10362 ]

@firebird-automations
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Modified by: @asfernandes

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

resolution: Fixed [ 1 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pcisar

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

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

QA Status: No test

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

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

QA Status: No test => Cannot be tested

@firebird-automations
Copy link
Collaborator Author

@firebird-automations
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Commented by: @ibprovider

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

@firebird-automations
Copy link
Collaborator Author

Commented by: @dyemanov

Runtime setting via IMaster sounds good for me.

@firebird-automations
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Commented by: @hvlad

Adriano,

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

@firebird-automations
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Commented by: @hvlad

Adriano,

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

@firebird-automations
Copy link
Collaborator Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment