Closed
Conversation
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
3 times, most recently
from
7d8ceb0 to
caebbf6
Compare
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
4 times, most recently
from
cbedeea to
14a43b0
Compare
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
2 times, most recently
from
3c8c362 to
a8b08eb
Compare
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
3 times, most recently
from
d0b751e to
2c8acef
Compare
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
from
16a74ef to
bd4b5f7
Compare
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
4 times, most recently
from
0fa4cae to
4994c13
Compare
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
from
ee8a1dc to
d39fb49
Compare
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
from
277885a to
bd3ff54
Compare
dorotaphanSiili
force-pushed
the
dphan/add_android
branch
3 times, most recently
from
d50a50c to
5d380fa
Compare
alwa-siili
force-pushed
the
dphan/add_android
branch
from
7da22c2 to
b2b5f5c
Compare
alwa-siili
force-pushed
the
dphan/add_android
branch
from
8febc46 to
8d2a405
Compare
alwa-siili
force-pushed
the
dphan/add_android
branch
from
8d2a405 to
f852122
Compare
Add all the new module files, updates the plugins with new module. The service stub is minimal empty implementation that does not yet handle any jni. It can be created, allows setting a backend, it provides empty methods common for all service adapters.
Adds gradle files necessary for the android build, adds ue4shared.keystore also needed for the android app to be run and the jni_UPL.xml which allows adding the configuration for unreal generated xml file the upl does not yet have necessary configuration
Fill in the upl file to copy necessary java modules. The modules must be placed inside the plugin directory in the android folder. This code is generated by java template. The also expose the services provided by the java code, adds permission for binding, adds queries (this part is necessary for clients). All the dependencies for includes and modules are also added.
Adds the code generated for the jni implementation for backend. It requires implementing native functions. For now they are added to service adapter but without implementation.
Notification with async task - if not in the game thread, was already covered in earlier task. This functionality in jni adapters is no longer required.
jclass references are global references that needs to be deleted when no longer used. It is also costly operation to get those, and they best be stored as statics, searched once, and reused. With our current build model, the java libs are not dynamic, they don't change, so once loaded it is safe to use them through the lifetime of the program. There is an issue when to initialize them. The startup of the module is too early (it is cleaned up at the shutdown though). It may be introduced in the module without any interface (structs and enums only module), also it must be ready when any other module want's to use it. Hence I introduced the one-time thread-safe lazy initialization for those JNI resources. It must be checked on each function use.
Almost each jni call needs to be checked if any jni error occurred - one thing is that it crashes, other if no crash it blocks all next jni calls. This is costly operation, so best if, for obtaining resources that don't change it is performed once.
Commit introduces:
1. Caching the methodsIds and fieldIds - all those are marked static
consts in a function, initialized once with a function call.
2. Error handling:
- checking for jni error and clearing if necessary
- checking against nullptrs
The client and adapter class implement fixes as already added for the
data converters:
- jclasses are properly deleted when no longer needed
- all jni calls are checked for jni errors
- the non-changing resources are initialized only once. They are
statically stored in the cpp class and all get initialized at once.
In a ticket APIGEAR-372 there was suspicion of different handling conversion of strings with unreal jni helper. This tuned out to be not true (although 4.27 does not convert string arrays - there is different ticket to align to 4.27 APIGEAR-401). All versions convert the strings but putting it in wrappers that manage local ref lifetime. For template facilitation we in most cases we make the local ref anyway. Using the wrapped ones directly requires another ifs for using the variable, condition to check if local ref needs to be removed. Also it requires adding new handling for getters, which are not allowed to delete the local/scoped object as it needs to be passed to java. Hence the decision to make local ref for simpler handling. This may be changed. Anyway in data converter when handling the field of string type, there was local ref made, but never deleted. This was made in scope only for string handling, so it can easy be used with the wrapper only, no need for local ref, no need for removing it later then.
Native calls from jni are always called from jni thread. The Unreal's rule is that UObjects (including subsystems) should not be used from threads other than game thread for most usages, due to multithreading issues. But within current usage, and assuming: - Adapter is an EngineSubsystem which lifetime is guaranteed for application lifetime - when jni code is used, the editor is not used (jni code only runs on android devices, where editing blueprints is not possible) - The backend is UPROPERTY and is thread-safe subsystem itself. It is safety criteria are met. The calls need also an easy access to the subsystem. Although it is the jni adapter subsystem's responsibility to start and stop the java backend which implements those native calls, since the jni has its separate thread some race conditions cannot be excluded, especially during tear down. The subsystems itself won't be deleted or garbage collected, but the calls are not supposed to be used after deinitialize. To provide the easy and access and improve the thread safety, especially on teardown the std::atomic was added around the handle, with which jni native calls are using the subsystem. To give the native calls access to only thing they need I introduced a simple interface that is implemented by the subsystem. The intention was to inherit it privately, but it seems unreal Subsystem does not accept such form. The handle is assigned in initialize and reset in deinitialize, which prevents from using it outside the proper time slot. In adapter the native jni calls operate on currently set backend. The backend may change, but it is not expected to be frequent if at all, nevertheless it should be guarded with a mutex, so the usage won't occur while jni is asking for it. The already existing UFUNCTION to get the backend is not guarded with it, the assumption is that only game thread will change it, and that this function is used from game thread - this means that guarding it with mutex is not necessary. WARNING: how about calling methods on backend in case the methods are implemented in blueprints?
Similarly as in previous commit for adapter:
Native calls from jni are always called from jni thread.
The Unreal's rule is that UObjects (including subsystems) should not be
used from threads other than game thread for most usages, due to
multithreading issues. But within current usage, and assuming:
- using Abstract class as a base class, provides thread safe access to
data
- Publisher class allows thread safe usage
- client is a EngineSubsystem which lifetime is guaranteed for
application lifetime
- when jni code is used, the editor is not used (jni code only runs on
android devices, where editing blueprints is not possible)
... the safety criteria are met.
Also jni calls need easy access to the subsystem.
Although it is the jni client subsystem's responsibility to construct
and delete the java client which implements those native calls, since
the jni has its separate thread some race conditions cannot be excluded,
especially during tear down.
The subsystems itself won't be deleted or garbage collected, but the
calls are not supposed to be used after deinitialize.
To provide the easy and access and improve the thread safety, especially
on teardown the std::atomic was added around the handle, with which jni
native calls are using the subsystem.
To give the native calls access to only thing they need I introduced a
simple interface that is implemented by the subsystem.
The intention was to inherit it privately, but it seems unreal Subsystem
does not accept such form.
The added interface covers functionality of the Subscriber interface for
each defined interface in module yaml file, but it does not expose the
functions to blueprints, which is crucial here.
The handle is assigned in initialize and reset in deinitialize, which
prevents from using it outside the proper time slot.
The commit moves closer making local copy of handle and its usage, since
deinit may happen between those, but calling methods is still valid on
that copy: subsystem is alive, still has data to be changed (in case of
properties), publisher is alive and thread safe.
WARNING: what happens if the properties are referenced interfaces?
Currently, the 'In' prefix isn't used by signal parameters. It can lead to issues, such as: error C4458: declaration of 'Param' hides class member. Having the class member named with `In' prefix is rather unlikely, since its an UE convention for input parameters. Use the 'In' prefix in signal parameters to prevent C4458 errors.
Now the service starer sends information about service lifecycle:
- notifies when the service successfully starts
- notifies when the service is killed, not if it is closed by
intention.
The service may get killed if it is stared in separate process and
android os decides it runs out of resources.
This commit adds handling to the service starter native method and uses
delegates to notify about such situation.
What is missing is a method to restart the service. It must be restarted
explicitly with a service starter.
So far, the JNI module is registered unconditionally in the uplugin files. Allign their registration with other modules (olink, modbus, etc).
This test dll handle is an leftover from old designs. Remove it.
Until now, getBackendService wasn't guarded by BackendServiceCS. However, theoretically, getBackendService can be called from any thread in c++ code. Make sure it's also guarded properly.
The {{$Class}}Cache class uses static jmethodID and jclass members that
are initialized in init() (called from game thread during Initialize())
and read from JNI threads (in native callbacks). While jmethodID values
are stable once obtained, there's no memory fence or synchronization
ensuring the JNI thread sees the initialized values. In practice, this
likely works due to implicit synchronization through the Java binding
process, but formally it's a data race.
Since we always initialize and deinitialize the whole Cache, make it a
thread safe shared ptr.
During initialize, read all methods and set the valid cache pointer.
During uninitialize, reset the pointer to null.
This also prevents from use-after-free bug.
We're leaking the reference to an enum object when converting the structure. Delete the local enum reference after using it.
The enum value is cast to uint8 before being assigned to int. If the API enum has values >= 256, this silently truncates. The JNI fromValue method accepts an int parameter, so there is no JNI-side reason for the truncation. Cast directly to int instead of uint8.
We exit early if during JniAdapter::Deinitialize() the BridgeClass isn't null, but the stop static function isn't found. This is rather an orthodox situation, however, it leads to a data leak. Technically speaking, if JniAdapter::Deinitialize() is called, the UE application is closing down, so the system will cleanup everything anyways. However, for the sake of correctness, always free the gobal reference and clear the cache, even if the stop method couldn't be found. Since we use service in the same process as the UE application (that's currently the only working solution), the service will be killed by the system anyways, when the application finishes.
In Jni{Adapter,Client}::Initialize, we initialize the Cache. However, in
JniAdapter::Initialize we don't check, whether the initialization was
successful.
Check during Cache::init whether all variables are correctly read and no
JNI exceptions happened. If anything happened, don't initialize the
cache. Then check, whether the cache is initialized during
JniAdapter::Initialize and bail if the cache wasn't initialized.
We're not checking whether m_javaJniServiceInstance is null in handling on<Property>Changed. Make sure we check for null safety.
alwa-siili
force-pushed
the
dphan/add_android
branch
from
1cb9070 to
1e5286c
Compare
Collaborator
|
Closing this PR, as it is superseded by #176. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
still missing:
Closes # https://jira.it.epicgames.com/browse/APIGEAR-276
📑 Description
✅ Checks
ℹ Additional Information