We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 1422 - Android SDL_rwops not thread-safe(?)
Summary: Android SDL_rwops not thread-safe(?)
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: file (show other bugs)
Version: HG 2.0
Hardware: ARM Android (All)
: P2 blocker
Assignee: Gabriel Jacobo
QA Contact: Sam Lantinga
URL:
Keywords:
: 1159 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-02-18 07:16 UTC by Ellie
Modified: 2012-07-20 13:28 UTC (History)
2 users (show)

See Also:


Attachments
Store JNIEnv locally in each thread (6.48 KB, patch)
2012-06-19 16:18 UTC, Gabriel Jacobo
Details | Diff
Better thread handling for Android (11.54 KB, patch)
2012-06-22 08:28 UTC, Gabriel Jacobo
Details | Diff
Better thread handling for Android (11.54 KB, patch)
2012-06-23 07:01 UTC, Gabriel Jacobo
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ellie 2012-02-18 07:16:04 UTC
Please close if invalid.

From the discussion in http://bugzilla.libsdl.org/show_bug.cgi?id=1159 and http://lists.libsdl.org/pipermail/sdl-libsdl.org/2012-February/083954.html and from own tests it seems that the Android SDL_rwops are not thread-safe.

My engine depends on threaded access to the packaged asset files and I cannot continue its port without this feature, therefore I'll choose 'blocker' - please change if not appropriate.

If this SDL_rwops thread-safety issue is truly present, when can we expect a fix? I do a lot of things with threads and I'd need lots of ugly hacks (e.g. block main thread constantly and make it read files for the sound thread or the graphics loader thread) to work around this issue which might in the end not work and cause a totally stuttering broken game, if SDL doesn't fix this.
Comment 1 Sam Lantinga 2012-02-20 20:59:41 UTC
My concern with the patch in bug 1159 is that it looks like it's attaching and detaching the java environment in each API call, which seemed extremely bad for performance.  Does the approach taken there fix the threading issues for you?

There are some active Android developers on the mailing list.  If you haven't recently broached this there, it might be a good idea to try it and see if anyone has suggestions on how to fix this.  I don't do enough JNI programming to know what the best fix is myself.
Comment 2 Sam Lantinga 2012-02-20 21:01:01 UTC
FYI, fixing this may be as simple as using a mutex inside the file functions to protect the data there, and if that's the case, this may be an easy fix.
Comment 3 Ellie 2012-02-21 02:26:21 UTC
Would a mutex allow only one file access at once?

My sound thread keeps some files constantly opened (for streaming) and would therefore block everyone else forever if that was the case.
Comment 4 Ellie 2012-02-21 11:30:14 UTC
I attempted to use the patch from #1159. It causes a crash when opening a window which I think worked before - probably thanks to the many manual adjustments done by me because half of the patch failed with the current code and I had to guess a lot (and probably guessed wrong).

Even if that patch offers just a slow solution, what about adding it to SDL with a compile time switch that is disabled per default so that people who need threading, like me, can enable it explicitely, while all the others keep the performance?
Comment 5 Ellie 2012-03-06 12:30:03 UTC
I could fiddle around with the patch until maybe I get it to work. Would that be helpful to you?

I tried hacking around file access from other threads than the main thread now (in my application code) which resolves the issue. However, if you imagine an SDL sound callback where .ogg is decoded from a file on disk that is constantly streamed, you can imagine what a mess hacking this file access back into the main thread is.

Any sane solution, even if involving a performance penalty, would be appreciated. Anything in SDL would be faster (and more reliable) than my current application space attempt of redirecting all file access into the busy main thread and making the other threads wait for the result - which is a code and performance nightmare.
Comment 6 Ellie 2012-04-15 14:40:32 UTC
I tried a mailing list posting now, so far it hasn't attracted JNI-savvy people: http://forums.libsdl.org/viewtopic.php?t=8076

Have you considered dropping JNI altogether? It could probably resolve this and  it should be faster doing things the direct way. Android 2.3 is pretty much standard by now. I filed a separate enhancement bug for this: http://bugzilla.libsdl.org/show_bug.cgi?id=1466

Also I looked at some native NDK code in the Android docs and it looks nicer to me than the huge JNI wrapping stuff SDL is filled with right now.
Comment 7 Gabriel Jacobo 2012-05-29 07:59:59 UTC
A few comments on this issue from what I've researched...

- Calling AttachCurrentThread on an already attached thread is a no op, and is actually the recommended method of getting the thread-exclusive JNIEnv environment. So, the first thing will be removing the static mEnv in SDL_android.cpp and replacing it for a local variable obtained via AttachCurrentThread (this is actually what I think is causing problems right now, at some point a thread is trying to use another threads JNIEnv)

- It seems you can have multiple threads attached at the same time, I haven't seen any mention of this being an exclusive sort of operation

- Threads have to be detached before they end, and that can be done almost automatically using pthread_key_create...how exactly I don't know :)

References:

http://developer.android.com/guide/practices/design/jni.html
https://groups.google.com/forum/?fromgroups#!topic/android-ndk/vLt3ncT3I4g
http://java.sun.com/docs/books/jni/html/invoke.html



Sam, let me know your thoughts on this, I can try to create a patch based on this asumptions and see if that solves the threading problems we've been seeing.
Comment 8 Gabriel Jacobo 2012-06-19 16:18:46 UTC
Created attachment 881 [details]
Store JNIEnv locally in each thread

Can anyone try this patch and see if it solves the problems? Thanks!
Comment 9 Gabriel Jacobo 2012-06-21 07:55:40 UTC
*** Bug 1159 has been marked as a duplicate of this bug. ***
Comment 10 Gabriel Jacobo 2012-06-22 08:28:47 UTC
Created attachment 885 [details]
Better thread handling for Android

Attaching improved patch
Comment 11 Gabriel Jacobo 2012-06-23 07:01:26 UTC
Created attachment 887 [details]
Better thread handling for Android
Comment 12 Gabriel Jacobo 2012-07-09 14:09:55 UTC
Evgeny Miroshnichenko confirmed this solves the threading issues on Android, this is now changeset http://hg.libsdl.org/SDL/rev/17840f487124
Comment 13 Ellie 2012-07-20 13:28:32 UTC
Sorry, university kept me busy. I will check the results the next three days. Thanks for the fix :-) (assuming it will work for me)