u_cleanup() should close OS level mutexes.

Description

Calling u_cleanup() should clean up any OS level mutexes used by the ICU library in addition to the heap memory that it already handles.

Failure to clean up OS level mutexes could be an issue for any process that repeatedly DLL loads and unloads ICU. Any such process must already call u_cleanup() to avoid memory leaks, meaning that nothing new would be required on the application side.

A brief history:

  • ICU 63 and earlier, mutexes leak on library unloading.

  • ICU 64 mutexes do not leak, but there are order-of-destruction problems.

Activity

Show:
Jeff Genovy
May 27, 2019, 10:21 PM

Thanks Markus.

Yes, this ticket can be closed as fixed. Thank you again Andy!

I think it might be worth adding the test I created above to the CI builds – though that should be a separate ticket.

Markus Scherer
May 27, 2019, 8:58 PM

Jeff, can we close this ticket as fixed? Do you want to do so?

Jeff Genovy
May 25, 2019, 2:35 AM

Thank you Andy!

Unfortunately, there isn’t really an easy way to test this with the existing ICU tests, as they never unload the ICU DLLs during their execution at all.

For the ICU tests, they load the DLLs at process creation time, and then they never actually unload them. When the test process dies, the critical sections get free’d since the process is dead, so you can’t easily tell that anything was leaked.

We need to explicitly unload the ICU DLLs and keep the process around in order to see the leaks.

 

I created a simple test program that does just that, and run that under AppVerifier which will report any active critical sections when a DLL is unloaded.

Link to the code: https://gist.github.com/jefgen/560077b80c64a833495e0d6c657f2b3d

 

With ICU 63, you get the following output (showing the leaks):

 

 

Using the same test program with ICU builds from master (which has PR #655) there are no longer get any reports about leaks:

 

 

Stepped though the code with WinDBG, I verified that it does indeed walk through the linked list and free/destruct the Mutex object which frees the critical section.

So the leaked OS Critical Sections are no more with Andy’s changes! 😊

Andy Heninger
May 24, 2019, 10:32 PM

I think this is fixed with PR #655.

But there is no test. I have reassigned this ticket to Jeff, in case there is some easy way to test for leaked mutexes on Windows. The regular tests (intltest, cintltst) should be clean on exit.

Fixed
Your pinned fields
Click on the next to a field label to start pinning.

Assignee

Jeff Genovy

Reporter

Andy Heninger

Components

Priority

major

Time Needed

Days

Fix versions