-
Notifications
You must be signed in to change notification settings - Fork 89
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
Feature/599 provide and use c service in rust #627
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #627 +/- ##
==========================================
+ Coverage 81.61% 81.63% +0.02%
==========================================
Files 260 260
Lines 34680 34679 -1
==========================================
+ Hits 28303 28311 +8
+ Misses 6377 6368 -9
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This reverts commit e3aa83a.
…d-use-c-service-in-rust
Just finished the book, hopefully that was not too late for our Rust party.
|
[lib] | ||
name = "celix" | ||
path = "src/lib.rs" | ||
crate-type = ["rlib"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it's OK to have static libcelix.
Considering its size, 200KB for x64 release build, it might be preferable to built as shared object.
But unfortunately I've not found any way to make BUILD_RPATH work for Corrosion, without which it is quite inconvenient to run the resulting binary in the build tree.
Even if we can add BUILD_RPATH
via build.rs, I don't know how to make RPATH rewrite work.
…rvice-in-rust' into feature/599-provide-and-use-c-service-in-rust
|
||
#[doc = "A trait to implement a Celix Shell Command"] | ||
pub trait RustShellCommand { | ||
fn execute_command(&self, command_line: &str) -> Result<(), Error>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&self
or &mut self
? What if a command execution changes the internal states?
So we have to use a dummy RustShellCommandImpl
to hold a mutable reference to the object we really want to change its states with such a command execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs IMO some more experimenting and/or discussion.
But a &self
argument - so not mutable - in a (service) trait can be used to change a service state, but it requires the the implementing object uses a Mutex to change the state.
An other option could be to register a services as Arc<Mutex> or maybe require that a service template argument is Sync
.
But I think this is something to get more clear later.
let include_path_file = build_dir.join("include_paths.txt"); | ||
file = File::open(&include_path_file)?; | ||
} else { | ||
println!("include_paths.txt not found in CORROSION_BUILD_DIR. Failing back to CELIX_RUST_INCLUDE_PATHS_FILE env value"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the message expresses the real intention, then the currently implementation, which fails back to CELIX_RUST_INCLUDE_PATHS_FILE
if CORROSION_BUILD_DIR
is not set, is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentionally, but does not really work. So I will remove the else branch.
#Note for now this includes framework, utils and shell_api maybe this should be separated in the future. | ||
file(GENERATE OUTPUT | ||
"${CMAKE_CURRENT_BINARY_DIR}/include_paths.txt" CONTENT | ||
"$<TARGET_PROPERTY:framework,INTERFACE_INCLUDE_DIRECTORIES>;$<TARGET_PROPERTY:utils,INTERFACE_INCLUDE_DIRECTORIES>;$<TARGET_PROPERTY:shell_api,INTERFACE_INCLUDE_DIRECTORIES>;$<TARGET_PROPERTY:Celix::log_service_api,INTERFACE_INCLUDE_DIRECTORIES>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INCLUDE_DIRECTORIES
alone is not enough, we shall also have COMPILE_DEFINITIONS
and COMPILE_OPTIONS
.
@@ -44,19 +58,15 @@ fn main() { | |||
println!("cargo:info=Start build.rs for celix_bindings"); | |||
let include_paths = print_include_paths().unwrap(); | |||
|
|||
let mut builder = bindgen::Builder::default() | |||
.header("src/celix_bindings.h"); | |||
let mut builder = bindgen::Builder::default().header("src/celix_bindings.h"); | |||
|
|||
// Add framework and utils include paths | |||
for path in &include_paths { | |||
builder = builder.clang_arg(format!("-I{}", path)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned earlier, -I
is not enough, we may need -D
and other compile options (like -pthread
).
It is hard to tell whether it matters for now without checking each target's INCLUDE_DIRECTORIES
, COMPILE_DEFINITIONS
and COMPILE_OPTIONS
.
@@ -44,19 +58,15 @@ fn main() { | |||
println!("cargo:info=Start build.rs for celix_bindings"); | |||
let include_paths = print_include_paths().unwrap(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misisng cargo:rustc-link-lib=
leads to no DT_NEEDED for framework
and utils
shared objects.
@@ -44,19 +58,15 @@ fn main() { | |||
println!("cargo:info=Start build.rs for celix_bindings"); | |||
let include_paths = print_include_paths().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing parse_callbacks(Box::new(bindgen::CargoCallbacks))
so that changed Celix C headers will not lead to rebuild of this crate.
@@ -30,3 +30,6 @@ | |||
#include "celix_framework.h" | |||
#include "celix_framework_factory.h" | |||
#include "celix_framework_utils.h" | |||
|
|||
#include "celix_shell_command.h" | |||
#include "celix_log_service.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now all components (including framework
and utils
) are optional, these headers may be missing.
For Conan, intra-package and inter-package dependency deduction work like charm. But we need to think about how to make Cargo and Conan work together.
|
||
impl Debug for Error { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
match self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is celix_strerror
helpful for more useful Debug information?
|
||
pub struct LogHelper { | ||
name: String, | ||
tracker: Mutex<Option<ServiceTracker<celix_log_service_t>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With LogHelper
as the sole owner, the use of Mutex
is weird, whose purpose is to suppress compiler's complain of helper.tracker.lock().unwrap().replace(tracker)
?
}); | ||
let filter = format!("(name={})", name); | ||
let weak_helper = Arc::downgrade(&helper); | ||
let tracker = ctx.track_services::<celix_log_service_t>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the weirdness of the tracker mutex comes from this: providing a half-constructed object (log helper) to another object (tracker).
If we permit a new log helper holding a None tracker and opening the tracker after new, then LogHelper::new
returns Self rather than Arc<Self>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for contributing this eye-opening PR.
Reviewing this PR is not an easy task: familiarizing oneself with Rust's ownership model is not easy, and fitting it with the Celix's service layer which manages dynamic service lifetime is challenging. I was struggling with aligning these two properly. Maybe I need more time to absorb Rust. Never mind. I'll keep looking at the Rust part afterwards.
Let's get this cool addition merged in its current form and get 2.4.0 released ASAP.
handle: *mut ::std::ffi::c_void, | ||
_ctx: *mut $crate::details::CBundleContext, | ||
) -> $crate::details::CStatus { | ||
let reclaimed_activator = Box::from_raw(handle as *mut $activator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not feel right. In the C version, activator is dropped after celix_bundleContext_waitForEvents(ctx)
.
IMHO, the best fix is to change the corresponding logic in celix_framework_stopBundleEntryInternal
:
From
status = CELIX_DO_IF(status, bundle_getContext(bndEntry->bnd, &context));
if (status == CELIX_SUCCESS) {
if (activator->stop != NULL) {
status = CELIX_DO_IF(status, activator->stop(activator->userData, context));
if (status == CELIX_SUCCESS) {
celix_dependency_manager_t *mng = celix_bundleContext_getDependencyManager(context);
celix_dependencyManager_removeAllComponents(mng);
}
}
}
if (status == CELIX_SUCCESS) {
if (activator->destroy != NULL) {
status = CELIX_DO_IF(status, activator->destroy(activator->userData, context));
}
}
if (bndEntry->bndId >= CELIX_FRAMEWORK_BUNDLE_ID) {
//framework and "normal" bundle
celix_framework_waitUntilNoEventsForBnd(framework, bndEntry->bndId);
celix_bundleContext_cleanup(bndEntry->bnd->context);
}
To
status = CELIX_DO_IF(status, bundle_getContext(bndEntry->bnd, &context));
if (status == CELIX_SUCCESS) {
if (activator->stop != NULL) {
status = CELIX_DO_IF(status, activator->stop(activator->userData, context));
if (status == CELIX_SUCCESS) {
celix_dependency_manager_t *mng = celix_bundleContext_getDependencyManager(context);
celix_dependencyManager_removeAllComponents(mng);
}
}
}
if (bndEntry->bndId >= CELIX_FRAMEWORK_BUNDLE_ID) {
//framework and "normal" bundle
celix_framework_waitUntilNoEventsForBnd(framework, bndEntry->bndId);
celix_bundleContext_cleanup(bndEntry->bnd->context);
}
if (status == CELIX_SUCCESS) {
if (activator->destroy != NULL) {
status = CELIX_DO_IF(status, activator->destroy(activator->userData, context));
}
}
So that we can remove celix_bundleContext_waitForEvents(ctx)
from the generated celix_bundleActivator_destroy
. Also we re-promote usage of create/start/stop/destroy rather than CELIX_GEN_BUNDLE_ACTIVATOR
.
} | ||
|
||
pub struct ServiceTracker<T> { | ||
ctx: Arc<BundleContext>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not feel right, since BundleContext
has longer lifetime than any tracker associated with it.
This weirdness leads to unnatural BundleContext::get_self
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lifetime Order: BundleContext > Activator > Loggerhelper > ServiceTracker ...
If we can enforce such order using Rust type system, then lots of weirdness will disappear.
); | ||
} | ||
Err(e) => { | ||
println!("Error creating CString: {}", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use eprintln!
rather than println!
?
handle: &mut self.shell_command_provider as *mut CShellCommandImpl as *mut c_void, | ||
executeCommand: Some(CShellCommandImpl::call_execute_command), | ||
}) | ||
.with_service_name("celix_shell_command") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the current implementation, I can tell if with_service_name
comes before with_service
, an dynamic memory allocation will be avoided.
Thanks. I agree with a 2.4.0 ASAP. I think it is maybe better to merge this PR after 2.4.0. Then some additional work can be done to prevent/refactor some strange constructions (e.g. the log_helper <-> service tracker). |
I agree. Improving the C implementation and the Rust design at the same time may produce the best results. |
Hi, awesome to see Celix is making progress towards Rust 👍 Are you open to contributions on this topic? Although I am not completely familiar with the internals of Celix yet, I would love to wrap my head around your concepts and see if we can create a nice and secure Rust API for Celix. |
Hi, Although this pull request has remained dormant for some time, I still believe that introducing Rust bindings, alongside the existing C++ bindings, would greatly benefit Apache Celix. Please note that Rust support, as well as this pull request, is still experimental. Rust is a new for me (and, I believe, for the other committers as well), and we are all in the process of understanding how Rust concepts can be integrated into Apache Celix, or more broadly, how they fit within a dynamic, in-process, service framework. Another point to consider is that, for now, we have decided to develop Rust support on top of the C framework, thereby leveraging the existing and stable C framework. Theoretically, this approach could be reversed, but in my opinion, that is not our current objective. A good way to start would be to try out Apache Celix and, specifically, this branch. If you have any questions, feel free to ask them here or on the Apache Celix dev mailing list (see https://celix.apache.org/support/mailing-list.html). And, of course, it is possible to create a pull request on top of this pull request to introduce some changes. Apache Celix is part of the Apache Software Foundation (ASF), and in practice, this means that we can accept some small (code) donations, but for more or larger donations, an ASF Contributor Agreement is needed. |
Hi, thank you for the detailed explanations! I aswell think that Rust would really benefit Celix. I think the existing C API is a great starting point for now, as Rust interopperates usually well with C. As you already showed :) For now I would start with making myself more familiar with Celix. I was already able to develop a basic understanding of most of the concepts of Celix. But I definitely need to further enhance this. The next step would be to get a better understanding of the invariants that need to be upheld when using the C API. I saw, that a lot of these are already documented, but I assume there are more and maybe also implicit ones. For now I think this is already a lot to do. Let's see where this leads me. :) In any case, an ASF Contributor Agreement will not be an obstacle. Topics I stumbled upon but which I have put in my backlog for now: Allocation: by default allocating and deallocating on different sides of the ffi might lead to undefined behaviour. There are several possible solutions to this
Type conversion: There are various types in the C API that are similar to Rust types but can not be used as smoothly without further adjustments on the Rust side. For example the
|
Sound like a plan 👍 .
Trying Apache Celix using version 2.4 is fine, but generally speaking the master branch is also quite stable for exploring Apache Celix.
Great to hear, when this is needed I will give an additional trigger.
Good to hear, you are already seeing some topics and I think the topics are a good starting point. I was not aware that a separate way of mem allocation was needed/desired. But as stated, I am not a Rust expert, so help is welcome :) I will also try to eventual get this PR merged. Not as a stable or even completely usable addition, but then we at least have a Rust starting point in the master branch. But I am currently focusing on something else, so I am not sure yet when I have to pick up this PR. |
I wanted to give a little update on my progress :) I was able to get your POC running with the latest master branch. Only some small changes were necessary. However there were some error messages I could track down to filter compilation. In I also made some progress towards the API and wanted to share my thoughts. I heavily thought about the I think most important invariant. POC approach using Weak and Arc Approach using Arc<Mutex<Option>> Then I thought about using lifetimes. The problem with lifetimes is that they need a scope where the Owner lifes. But we have two (four) functions that are called after each other from the framework. The compiler does not know that these are called one after another. Also we convert the First lifetime approach: Static services In the stop function the user simply takes its user data from the activator and undos what he did in the start function. Second lifetime approach: Spawn a thread In the following repository I show how I think this could look like: I hope I was able to make everything clear to you. |
Thanks for the update.
You are correct, the usage of But I agree, ideally this is improved using Rust lifecycles. The coming week I will not have the time have a look at the code and give feedback, but when I have to time I will pick this up. |
Two weeks have passed and I wanted to share again my progress :) BundleContext lifetime However I postponed this topic a little bit as I think there are a few feasible solutions. We can later review these solutions and settle on one of them. Services So I think we have two options here:
How can we achieve ABI and API checks at runtime? I think we can utilize this crate to provide safe services. How could this look like? When another bundle now wants to use the service it must statically link against the API crate as well. When using the service it is checked that the type information and abi_stable library information are compatible. Additional opportunities Final hurdles Summary |
Very good, keep up the pace :) I did have a look at the implementation, but was struggling to find some spare time. |
In my opinion I would be great if the BundleContext and everything created from the bundle context can be controlled by lifecycles. This could ensure that we can have static checks for correctly handling service registrations and service usage within the lifecycle of a bundle. For now I also think it is fine to do this with spawning threads, but ideally a thread is not needed. I have experience with complex applications using about 100 bundles and this would also mean a 100 thread (including a 100 extra stack stack) purely for lifecycle management. If I understand it correctly a thread is used to ensure a object lifecycle from BundleActivtor::start until BundleActivator::stop. Is it not possible to somehow create a rust object that outlives the celix C framework, maybe even using a custom rust celix launcher and use that rust object to store "bundle lifecycle control" storage objects? |
This is indeed close to what C++ is doing and that case a |
Exactly, the C++ solution is where I took inspiration from. But by doing it with an |
First of, my complements about taking API and ABI compatibility into account. For C and C++ we currently only support source compatibility, so everything should be build with the same compiler using the same sources (include service interface headers). But the idea to do some API compatibility checking is not new. Apache Celix also has a An idea back when libffi was first created, was to use the I still think this would be a very welcome addition to Apache Celix, but I would still require significant effort to develop, test and make user friendly.
I have a short look at abi_stable and mainly looked for the license. Maybe I missing something, but I could not find under which license the code is provided. But this does indeed sound like a good addition for runtime compatibility checking.
Nice, I will have a deeper look into this. Currently when combining C and C++, it possible to use C service in C++ but not the other way around. If rust service can be more flexible that would be great. |
Indeed. The integral dynamic nature of dynamic services in Apache Celix is that they can come and go and users should be protected against misuse as much as possible. For C / C++ this is arranged by providing a "use service" API based on callbacks so that the framework can ensure services are kept "alive" during callbacks. But it can also be done by service trackers combined with set/add/remove callbacks and in that case the user is responsible for correctly using the callbacks and protection service usage. I agree that ideally in Rust this should be done in a safer way, but lack to rust experience to know how or even if this is feasible.
No worries, I find very interesting to read up your thoughts, ideas and POCs. IMO communication through PR comments are ok :) |
Intro
This PR completes the Rust feasibility issue (#599) by providing a proof of concept (PoC) implementation for Apache Celix with Rust.
I invested more time in this than I had initially planned. However, as a result, this PoC IMO demonstrates nicely the feasibility of integrating Rust support into Apache Celix.
It's good to highlight that this implementation serves as a proof of concept and is not ready for production. The code lacks consistent error checks, documentation, unit tests, and is not feature-complete.
Changes
In this PR, the following are introduced:
celix
crate, which builds upon thecelix_bindings
crate, offering a Rust-native API for:Erno
enum moduleBundleActivator
moduleLogHelper
moduleBundleContext
moduleBundleContext
module provides the following features:LogHelper
uses a service tracker to fetch an on-demand Clog_service
.RustShellCommand
type.BundleActivator
BundleContext::use_service
to access both the Rust and C shell commandsThe
rust_shell_cnt
Apache Celix container/executable can be used to try-out Rust integration. With thequery -v
command it should give a nice overview of provided and used service in Rust.Interesting Observations
Rust's templating system, especially its type inference capabilities, is quite powerful. However, working with templates and traits can be - based on my experience - complex. The is mainly because traits aren't complete types. As a result, template methods cannot seamlessly move a provided trait type from a Box (pointer) to a different container type, like Arc (atomic reference count (shared_ptr)). My assumption is that this constraint is because of potential slicing and memory allocation/deallocation challenges. But, I am still learning the ins and outs of Rust, so maybe more is possible. Consequently, for this PoC, I decided to make type containers (e.g., Box, Arc, Rc) part of the service type. As result, from a Rust perspective, one does not register a 'RustShellCommand' service type but an
Arc<RustShellCommand>
service type.While integrating Rust support into Apache Celix is feasible, I'm uncertain whether it's appropriate to include it directly within the Apache Celix main git repository, especially when leveraging CMake Corrosion. Though this method functions, in my experience is does not really integrate well with an IDE.
Lastly, Rust, would arguably be a better fit, if Apache Celix could support both Static Bundles and Shared Object Bundles, in addition to Zip Bundles (#94).