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

[jni] Add a call to jvm.DetachCurrentThread when the thread detaches #1163

Merged
merged 14 commits into from
May 22, 2024

Conversation

HosseinYousefi
Copy link
Member

Closes #557.

Copy link

github-actions bot commented May 21, 2024

PR Health

Changelog Entry ✔️

Details
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ✔️

Details
File Coverage

This check for test coverage is informational (issues shown here will not fail the PR).

License Headers ✔️

Details
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/ffi/example/main.dart
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/config_provider/config_spec.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_cli/test/model/checksum_test.dart

Package publish validation ✔️

Details
Package Version Status
package:ffi 2.1.2 already published at pub.dev
package:ffigen 13.0.0-wip WIP (no publish necessary)
package:jni 0.9.3-wip WIP (no publish necessary)
package:jnigen 0.9.1 already published at pub.dev
package:native_assets_builder 0.7.0 already published at pub.dev
package:native_assets_cli 0.6.0 already published at pub.dev
package:native_toolchain_c 0.4.2 already published at pub.dev
package:objective_c 1.0.1 already published at pub.dev
package:swiftgen 0.0.1-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@HosseinYousefi HosseinYousefi marked this pull request as ready for review May 21, 2024 15:27
@HosseinYousefi HosseinYousefi marked this pull request as draft May 21, 2024 16:03
@coveralls
Copy link

coveralls commented May 21, 2024

Coverage Status

coverage: 90.986% (-4.0%) from 94.98%
when pulling 4f04ef0 on detach-current-thread
into 652badc on main.

@HosseinYousefi HosseinYousefi marked this pull request as ready for review May 21, 2024 16:53
Copy link
Member

@mkustermann mkustermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With comments addressed: lgtm

pkgs/jni/src/dartjni.h Outdated Show resolved Hide resolved
pkgs/jni/src/dartjni.h Outdated Show resolved Hide resolved
pkgs/jni/src/dartjni.h Outdated Show resolved Hide resolved
pkgs/jni/src/dartjni.c Outdated Show resolved Hide resolved
pkgs/jni/src/dartjni.h Outdated Show resolved Hide resolved
@@ -146,6 +150,11 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL, // handle to DLL module
// Return FALSE to fail DLL load.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sidenote: This DllMain is assuming the C code in package:jni gets compiled into it's own DLL (?). That would prevent statically linking it into a bigger library (which may have already a different DllMain). It's a little bit of a smell I think.

If we can use C++ here (if not: why?) then a global C++ object can do things in it's constructor and destructor (e.g. initializer locks and free them, create TLS keys, free them, ...) that get run on shared library load & unload times.

(Arguably this doesn't solve the issue of detaching the thread from java at exit)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use C++. We would have to wrap the functions we want to call from Dart in extern "C" but otherwise it should not be a problem. We also generate another C file from jni.h which wraps the API and does things like conversion to global refs and exception checking.

In general, I did not really refactor the C code we had since 2 years ago. I think it's worth it to open an issue and do this in the near future. #1169

@HosseinYousefi HosseinYousefi merged commit 413dd27 into main May 22, 2024
26 checks passed
@HosseinYousefi HosseinYousefi deleted the detach-current-thread branch May 22, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When our JNI support attaches to the current thread, it should have a balancing DetachCurrentThread
3 participants