-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
crash with many threads in java #4231
Comments
|
Author: unicoletti |
|
Author: unicoletti |
|
Author: nbrachet
Not specifically but it first happened with the -server option (which turns UseParallelGC on right?). I also reproduced on a default (-client no UseParallelGC) JVM. I am running 64-bit JVM. Not that it should matter. |
|
Author: unicoletti
I think that starting from 1.6 -server does just that.
|
|
Author: rouault |
|
attachment http://trac.osgeo.org/mapserver/attachment/ticket/4231/diff : |
|
@nbrachet if you could you open a pull request with the proposed patch I will pull it in in time for inclusion in 6.2. |
…ther platforms still vulnerable. #4231
|
I have applied the patch for gcc for the time being. |
|
@unicolet I am following this issue now. Let me know here when you have an update (as reported on the -dev mailing list). |
|
@unicolet I've cloned your 'windows_ref_count_atomic' repoository. I get many many errors when I try to compile that on windows, such as: |
|
@unicolet latest pull compiles on Windows with no issues. Java/CSharp/PHP/Python mapscripts also compile. |
|
Instructions for testing the fix: build mapserver and java mapscript remember to use --with-threads options when configuring: On windows you probably have to use nmake instead of make. if the program runs to its completion w/o errors or crashing then it is considered to be successful. |
|
|
@jmckenna the error above is because java can't load javamapscript.dll due to missing dependencies: could you make sure that javamapscript.dll is in the mapscript\java directory, that all dependencies are met (ie they are in path) and then run again: |
|
I've done as you requested. Results below: |
Implements locking in reference counting code #4231
|
Setting PROJ_LIB inside the mapfile allows us to step over that issue (the issue of Java MapScript not using the PROJ_LIB env variable). Next results are: That querying is still going........... |
|
Latest results from pull: stuck, no cpu activity.. |
|
The fix for GCC has been pushed to master. |
|
undefined ref to __sync_sub_and_fetch this is with: gcc (GCC) 4.1.2 20070626 (Red Hat 4.1.2-13) |
|
@tbonfort I have worked around it and added a configure check for it, it's probably safer. It's on my fork in the windows_ref_count_atomic branch. Diff: https://github.com/unicolet/mapserver/compare/windows_ref_count_atomic Could you test it? |
|
@unicolet , this was reported to me by a client, I have no means to test this myself. |
|
@tbonfort perhaps then you could ask your client? |
|
The issue is fixed on all platforms where GCC is used and __sync_fetch_and_add is available. |
|
I have a fix for Windows (it builds and runs with VS Express 10), could anyone test it before I merge it? |
|
I've tried to build Java mapscript on Windows (MSVC 2008). Using branch 'windows_ref_count_atomic' I get the following errors: mapscript_wrap.c(1498) : warning C4090: 'function' : different 'const' qualifiers Stop. |
|
@unicolet I have updated your 'windows_ref_count_atomic' branch and here are my results of the threadtests: |
|
looks good, will merge later today
|
fix MapServer#4231 for Windows too
|
__sync_sub_and_fetch is still causing trouble, at least with gcc 4.1.2.1: this seems spot on: http://trac.wxwidgets.org/ticket/4542 |
|
@tbonfort I'm surprised this has not been caught by the configure step since it no longer depends on compiler version, but will instead try to compile a small program against it. I'll check on it asap. |
|
Closing: definitely fixed in pull request 4502 |
fix MapServer#4231 for Windows too
Reporter: nbrachet
Date: 2012/03/09 - 02:09
Trac URL: http://trac.osgeo.org/mapserver/ticket/4231
'''How to reproduce:'''
Modify
`````` MapThread.java
to remove the call toSystem.gc()```. For this problem to reproduce one thread has to delete the mapObj object, while simultaneously another one deletes a layerObj object (from the same mapObj object), which happens when garbage collection runs.
Run with many threads for a while... because this is a threading issue it can work for a while without problem. In our testing we would crash within a half hour on a 8-core machine running 20 threads with JRE 1.6.
'''Analysis:'''
Reference counting isn't thread-safe.
layerObj objects obtained from a call to mapObj.get(int) are really shared between the layerObj and the mapObj objects. When garbage collection runs it may actually delete the layerObj object simultaneously from 2 different threads: one deleting the layerObj, the other one deleting the mapObj.
freeLayer()calls
MS_REFCNT_DECR_IS_NOT_ZERO(), itself implemented in terms of
MS_REFCNT_DECR()which is a simple macro around operator++... not thread-safe.
In some (rare) case both threads think the ref counter went to zero because of their own call to
MS_REFCNT_DECRand they end up both calling free() causing the crash.
'''Solution:'''
A simple solution is to use atomic int operations,
__sync_fetch_and_add/
__sync_sub_and_fetchfor GCC,
InterlockedIncrement/
InterlockedDecrementon WIN32, etc...
Attached is a patch against 6.0.2 that implements this change for GCC.
The text was updated successfully, but these errors were encountered: