Draft: [GSoC 2024]: extensions: Add Locking and Access Functions for Managing Extension Array

This MR introduces new functions for synchronization and access control in the extension manager. These updates are part of the ongoing effort to integrate Rust with VLC. The changes address the following issues:

• Ownership Flexibility: When the extension manager is instantiated from C, it directly owns the extensions. However, if the manager is created from Rust or another language, that language retains ownership of the extensions, which can result in the extension array within the manager being null. This flexibility is crucial for integrating Rust, as it allows Rust code to manage and own the extensions while ensuring compatibility with existing C code.

• Synchronization and Access Control: To handle these scenarios effectively, the new functions provide necessary synchronization and access control mechanisms. This ensures that the extension array is managed consistently, regardless of the language used to instantiate the manager.

added MRStatus::Reviewable label

When the extension manager is instantiated from C ... if the manager is created from (...) another language,

The extension manager is never instantiated from C so that makes zero sense, TBH.

Also using array indexing while not keeping the mutex does not make much sense. The argument that this is a preexisting issue only flies if you do not build upon that preexisting issue.

So frankly, I find this MR very disagreeable.

The extension manager is never instantiated from C

By that, I meant that when the ownership of extensions lies with the extension_manager_t, managing the lifetime of extensions and their corresponding sys becomes a bit diffilcult from other languages. Poor choice of words before, I guess.

Without these changes, we would need to append extensions to p_extensions_manager->extensions, as done here. While this isn't a trouble, it makes the extension management less flexible and more tightly coupled with the C implementation. It becomes much easier if Rust has the full ownership of Extension and its sys in the true sense.

That way we can have control over the lifetime of sys (e.g WasmExtension in !5528) which was the true motivation behind this MR.

The current design is unsound and you can't control the extension lifetimes with it. Basically the current design is racy. AFAICT, there are no ways that this is going to work in Rust unless you paper over the safety issues with unsound unsafe code.

I fundamentally disagree with this MR.

AFAIU this change is exactly what fixes the unsound design from the current extension_manager.

With the current extension_manager, it's not possible to write a Drop implementation without dynamic dispatch or from directly casting the p_sys pointer from the extension_t inside the CapabilityTrait implementation, which shows why this could be unsound.

With this MR, it's possible to allocate the final extension type inside the module itself, and share the references to the hidden internal extension descriptor descriptor directly. That way, it's possible to write without a single unsafe regarding extension management on the rust side, which would justify the local soundness.

Regarding the implementation itself, I agree that the transition to the new interface should be improved. I'd organize like the following:

• adding the new API in vlc_extensions like you did, but with vlc_mutex_lock( &p_mgr->lock ); and unlock being called on Lock/Unlock directly.
• changing the old clients to use the new API
• remove the lock and the old API (exposed array)

@alexandre-janniaux I have added the changes. Is this what you meant?

Sure, clearly. It moves the concurrency problem to the module itself, which is the best suited to address it since it owns all the data.

The alternative is finding another concurrency model where it would not have to race for operations (count / get), like maybe a callback mechanism.

IMO, if you want to be able to have a "dynamic" plugin list, I don't see how you can really avoid a callback mechanism just like service discovery :/

The plugin list is not dynamic in the extension providers, but regardless, yes, the same applies for a static plugin list with dynamic properties like "being enabled or not".

I'm not sure why I didn't think about that before, this is probably the correct interface for a futureproof extension system.

Then, we'd have a vlc_extensions_manager_cbs struct mapped to extensions_manager_t::cbs by the extensions_manager_t client. The callbacks would have:

• report_extension_added(id, extension_t*)
• report_extension_removed(id)
• report_extension_state_changed(id, bool enabled)

And the UI would be able to sync anything it wants from within the callback.

What would you think?

The plugin list is not dynamic in the extension providers

Then why do we need a mutex?

Edit: Also to answer your question, on the top of my head, I wouldn't be against the callback mechanism but since I've been ignoring anything extension related (on purpose shame on me) I might be missing critical issues.

Seeing vlc_mutex_init( &p_mgr->lock ); in lua/extension.c, I'd say that The Past always has its own answers that I cannot seem to find. If I had to take a guess, I'd say that initial design planned (or did, haven't looked the history) an autorun setup to start extensions from the beginning of the application, or planned multiple extension managers to be loaded, or initially tried to load the extensions for the extension thread, or maybe it was indeed a "reload" functionality (which is effectively unloading and reloading the extension module currently) but that was never implemented. No clue unfortunately.

added MRStatus::InReview label and removed MRStatus::Reviewable label

changed title from extensions: Add Locking and Access Functions for Managing Extension Array to [GSoC 2024]: extensions: Add Locking and Access Functions for Managing Extension Array

changed milestone to %4.0

added Component::Interface: other plugins label

added 1 commit

• 9b2bedc2 - extensions: Add Locking and Access Functions for Managing Extension Array

Compare with previous version

Preexisting issue but you are using then an extension from the array (wherever it is) outside of the lock.

AFAIU, all the struct members of extension_t are read-only, and the only member that can be modified (p_sys) should have a locking mechanism where required.

You seem to imply that your manager will be able to add or remove extensions dynamically, if so indexes and the address pointed by this index could theoretically change.

If the array is a oneshot array allocated in the open and never changed throughout the manager's life then there is no need for a mutex at all.

With the current implementation, the extension array is allocated only in the open function. Indeed there seems no use of Mutex for our case. The mutex might have been added for potential future use when extensions can be added dynamically.

also preexisting issue but you are using an index outside of the lock and nothing guarantees that it will be the same when triggerMenu is called.

added 1 commit

• 1d96edbb - extensions: Add Locking and Access Functions for Managing Extension Array

Compare with previous version

Conversely this also emphasize that we currently "only" have one extension manager retrieved uniquely by module_need if you want another one you're going to need to use module_match and monitoring several ones.

Conversely this also emphasize that we currently "only" have one extension manager retrieved uniquely by module_need if you want another one you're going to need to use module_match and monitoring several ones.

Yes, that's one of the blocking point in the GSoC, we had to have a higher priority than the lua module in order to test it. Some design is required to see whether we expose a unified extension manager object to the UI or load each dedicated extension manager from the UI themselves.

added 54 commits

• 1d96edbb...f20e1c2c - 53 commits from branch videolan:master
• d77a1f8e - extensions: Add Locking and Access Functions for Managing Extension Array

Compare with previous version

marked this merge request as draft