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

Race conditions with AMD and plugin hooks #7926

Closed
mrclay opened this Issue Feb 12, 2015 · 19 comments

Comments

Projects
None yet
3 participants
@mrclay
Member

mrclay commented Feb 12, 2015

How does a plugin use AMD and plugin hooks, but guarantee that its hook handlers are registered by the time they're triggered in another module?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 13, 2015

Member

Strawman: We define a module "elgg/hooks", which (somehow) has all active plugins as dependents. Modules that trigger hooks can only do it by depending on elgg/hooks.

Member

mrclay commented Feb 13, 2015

Strawman: We define a module "elgg/hooks", which (somehow) has all active plugins as dependents. Modules that trigger hooks can only do it by depending on elgg/hooks.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 13, 2015

Member

To participate, a plugin provides a module PLUGIN_ID/hook_handlers, which can depend on any module EXCEPT elgg/hooks. The elgg/hooks module will be defined via PHP to depend on all the plugin hook_handlers modules.

So now a module can require(elgg/hooks) with confidence that all hooks have been registered.

This creates a perf issue because defining elgg/hooks could mean always requiring a deep tree of mofules. But that seems to be the cost of implementing hooks in a lazy-loading async environment.

Member

mrclay commented Feb 13, 2015

To participate, a plugin provides a module PLUGIN_ID/hook_handlers, which can depend on any module EXCEPT elgg/hooks. The elgg/hooks module will be defined via PHP to depend on all the plugin hook_handlers modules.

So now a module can require(elgg/hooks) with confidence that all hooks have been registered.

This creates a perf issue because defining elgg/hooks could mean always requiring a deep tree of mofules. But that seems to be the cost of implementing hooks in a lazy-loading async environment.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 13, 2015

Member

Strawman 2: Any modules that trigger hooks could do something like this:

// views/default/js/elgg/module.php
define(function (require) {
    <?php echo elgg_inject_amd_deps($vars); ?>

    // use trigger hook...
});

elgg_inject_amd_deps() looks for $vars['amd_dependencies'] and if it's an array of strings, it returns something like:

require('string1'); require('string2');

Now any plugin can register dependencies on this module:

elgg_register_amd_dependency('elgg/module', 'myplugin/hooks');

Where this function sets up a handler for the "view_vars", "elgg/module" hook (PHP), where it sets $vars['amd_dependencies'][] = 'myplugin/hooks'.

Member

mrclay commented Feb 13, 2015

Strawman 2: Any modules that trigger hooks could do something like this:

// views/default/js/elgg/module.php
define(function (require) {
    <?php echo elgg_inject_amd_deps($vars); ?>

    // use trigger hook...
});

elgg_inject_amd_deps() looks for $vars['amd_dependencies'] and if it's an array of strings, it returns something like:

require('string1'); require('string2');

Now any plugin can register dependencies on this module:

elgg_register_amd_dependency('elgg/module', 'myplugin/hooks');

Where this function sets up a handler for the "view_vars", "elgg/module" hook (PHP), where it sets $vars['amd_dependencies'][] = 'myplugin/hooks'.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 13, 2015

Member

Simpler (and more dangerous): just allow plugins to prepend code (via view) to the body of the definition function.

// views/default/js/elgg/module.php
define(function (require) {
    <?php echo elgg_view('js/elgg/module/head'); ?>

    // use trigger hook...
});
Member

mrclay commented Feb 13, 2015

Simpler (and more dangerous): just allow plugins to prepend code (via view) to the body of the definition function.

// views/default/js/elgg/module.php
define(function (require) {
    <?php echo elgg_view('js/elgg/module/head'); ?>

    // use trigger hook...
});
@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Feb 13, 2015

Member

I thought we're currently not caching the AMD modules? (We cannot use PHP unless we cache the views.)

Member

juho-jaakkola commented Feb 13, 2015

I thought we're currently not caching the AMD modules? (We cannot use PHP unless we cache the views.)

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 13, 2015

Member

All AMD modules are loaded through /cache. In this case indeed these modules would have to be in .php files unfortunately.

Member

mrclay commented Feb 13, 2015

All AMD modules are loaded through /cache. In this case indeed these modules would have to be in .php files unfortunately.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 13, 2015

Member

I think my first idea was a lot better. It doesn't introduce require the plugin dev to know all the views that use a particular hook.

Member

mrclay commented Feb 13, 2015

I think my first idea was a lot better. It doesn't introduce require the plugin dev to know all the views that use a particular hook.

@mrclay mrclay added the critical label Mar 18, 2015

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 18, 2015

Member

@ewinslow got any input here?

Member

mrclay commented Mar 18, 2015

@ewinslow got any input here?

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Mar 19, 2015

Member

Ideas:

Make the rendering more React-like, such that plugins can load their modules lazily and the ui will auto update when the new hook is registered because the result of triggering the hook will be different. This is my preference but seems like not an option in many cases like ckeditor which doesn't seem like it can work like that (reinitializing with different options during editing).

Actually that's my only idea. The other one is the same as Steve's: prevent hook triggers until plugins have loaded their required modules.

Member

ewinslow commented Mar 19, 2015

Ideas:

Make the rendering more React-like, such that plugins can load their modules lazily and the ui will auto update when the new hook is registered because the result of triggering the hook will be different. This is my preference but seems like not an option in many cases like ckeditor which doesn't seem like it can work like that (reinitializing with different options during editing).

Actually that's my only idea. The other one is the same as Steve's: prevent hook triggers until plugins have loaded their required modules.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 19, 2015

Member

If our hooks API didn't exist, how would we implement the task of giving plugins the opportunity to filter values in an AMD a system?

Member

mrclay commented Mar 19, 2015

If our hooks API didn't exist, how would we implement the task of giving plugins the opportunity to filter values in an AMD a system?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 19, 2015

Member

An idea I had while writing the SO post: #8082

Member

mrclay commented Mar 19, 2015

An idea I had while writing the SO post: #8082

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 19, 2015

Member

The decoration solution is working! #8085

Member

mrclay commented Mar 19, 2015

The decoration solution is working! #8085

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 12, 2015

Member

I feel like this is important to solve as we push more folks into AMD. Need some fresh ideas. StackOverflow had nothin'

Member

mrclay commented May 12, 2015

I feel like this is important to solve as we push more folks into AMD. Need some fresh ideas. StackOverflow had nothin'

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue May 17, 2015

feature(javascript): Add plugin boot modules and element “behaviors”
Plugins can provide a <plugin_id>/boot AMD module which will be automatically
loaded by a new elgg/booted module. Plugin boot modules are the ideal
place to register plugin hooks, and the return values can be used to add
behavior to DOM elements (e.g. turn textareas into WYSIWYGs).

The elgg/booted module can be required to make sure all the plugin hooks
and behaviors are registered.

The final system hooks are now triggered in elgg/booted.

Fixes #7926, #6261
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 18, 2015

Member

#8319 has some ideas here

Member

mrclay commented May 18, 2015

#8319 has some ideas here

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue May 26, 2015

feature(javascript): Adds plugin boot modules
Plugins can provide an AMD module boot/<plugin_id>, which will be automatically
loaded by a new elgg/booted module. Plugin boot modules are the ideal
place to register plugin hooks.

The elgg/booted module can be required to make sure all the plugin hooks
have been registered.

The final system hooks are now triggered in elgg/booted.

This also adds a boot module for likes, and has elgg/ckeditor/config depend
on elgg/booted.

Fixes #7926
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jun 1, 2015

Member

Idea 5(?): Server-side we configure AMD module methods to be registered as client-side hook handles. A declarative API would be ideal:

// client-hooks.php
return [
  [
    'hook' => 'init',
    'type' => 'system',
    'module' => 'plugin2/hooks',
    'method' => 'init',
    'priority' => 100,
  ],
];

The result is a module elgg/booted containing a series of statements (in plugin order) like:

// from mod/plugin2/client-hooks.php
elgg.register_hook_handler('init', 'system', require('plugin2/hooks')['init'], 100);
// from mod/plugin5/client-hooks.php
elgg.register_hook_handler('config', 'something', require('plugin5')['alterSomething']);

The elgg/booted module is always required, but modules that consume hooks must depend on it explicitly (e.g.).

Benefits:

  • hook handlers are forced to be globally accessible (via require()) methods, which could allow us to add an unregister feature.
  • required modules are known server-side, which could aid in streamlining module loading
Member

mrclay commented Jun 1, 2015

Idea 5(?): Server-side we configure AMD module methods to be registered as client-side hook handles. A declarative API would be ideal:

// client-hooks.php
return [
  [
    'hook' => 'init',
    'type' => 'system',
    'module' => 'plugin2/hooks',
    'method' => 'init',
    'priority' => 100,
  ],
];

The result is a module elgg/booted containing a series of statements (in plugin order) like:

// from mod/plugin2/client-hooks.php
elgg.register_hook_handler('init', 'system', require('plugin2/hooks')['init'], 100);
// from mod/plugin5/client-hooks.php
elgg.register_hook_handler('config', 'something', require('plugin5')['alterSomething']);

The elgg/booted module is always required, but modules that consume hooks must depend on it explicitly (e.g.).

Benefits:

  • hook handlers are forced to be globally accessible (via require()) methods, which could allow us to add an unregister feature.
  • required modules are known server-side, which could aid in streamlining module loading
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jun 1, 2015

Member

Idea #8365: A plugin can provide a boot module with a standard name, and it uses a formal API (elgg/Plugin) to declare itself to the system. A module elgg/booted requires and initializes the plugins in plugin order.

Benefits:

  • No server-side config needed: drop boot module in expected place. If it declares an "init", it's initialized.
  • Plugin object (instead of, say, an POJO return) allows useful error messages for things like misspelling config keys and devs could look at this module to understand the system.
Member

mrclay commented Jun 1, 2015

Idea #8365: A plugin can provide a boot module with a standard name, and it uses a formal API (elgg/Plugin) to declare itself to the system. A module elgg/booted requires and initializes the plugins in plugin order.

Benefits:

  • No server-side config needed: drop boot module in expected place. If it declares an "init", it's initialized.
  • Plugin object (instead of, say, an POJO return) allows useful error messages for things like misspelling config keys and devs could look at this module to understand the system.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jul 2, 2015

feature(javascript): Adds plugin boot modules
Plugins can provide an AMD module boot/<plugin_id>, which will be automatically
loaded by a new elgg/booted module. Plugin boot modules are the ideal
place to register plugin hooks.

The elgg/booted module can be required to make sure all the plugin hooks
have been registered.

The final system hooks are now triggered in elgg/booted.

This also adds a boot module for likes, and has elgg/ckeditor/config depend
on elgg/booted.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 1, 2015

feature(javascript): moves hooks usage to plugin boot modules
Deprecates elgg.trigger_hook and elgg.register_hook_handler in favor of two new
AMD modules. These ensure all hooks are registered before being triggered:

- `elgg/Plugin` is a constructor for a plugin configuration object, and provides
`trigger_hook` as a static method.
- `elgg/booted` loads and initializes all plugin boot modules in priority order,
and provides `register_hook_handler`.

Plugin boot modules are named `boot/<plugin_id>` and return a Plugin instance.

The final system hooks are now triggered in `elgg/booted`.

Deprecates the client-side `boot, system` hook.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 1, 2015

feature(javascript): moves hooks usage to plugin boot modules
Deprecates elgg.trigger_hook and elgg.register_hook_handler in favor of two new
AMD modules. These ensure all hooks are registered before being triggered:

- `elgg/Plugin` is a constructor for a plugin configuration object, and provides
`trigger_hook` as a static method.
- `elgg/booted` loads and initializes all plugin boot modules in priority order,
and provides `register_hook_handler`.

Plugin boot modules are named `boot/<plugin_id>` and return a Plugin instance.

The final system hooks are now triggered in `elgg/booted`.

Deprecates the client-side `boot, system` and `init, system` hooks.

Fixes #7926
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 1, 2015

Member

PR #8993 adds boot modules, and the involved AMD modules provide the APIs to register and trigger hooks with proper blocking: You can only register in your plugin's boot module, and you can only trigger after all plugins are booted (otherwise deprecation warnings).

Member

mrclay commented Oct 1, 2015

PR #8993 adds boot modules, and the involved AMD modules provide the APIs to register and trigger hooks with proper blocking: You can only register in your plugin's boot module, and you can only trigger after all plugins are booted (otherwise deprecation warnings).

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 1, 2015

feature(javascript): moves hooks usage to plugin boot modules
Deprecates `elgg.register_hook_handler()` and `elgg.trigger_hook()` in favor
of dedicated `elgg/hooks/register` and `elgg/hooks/trigger` modules. These and
other new modules ensure all hooks are registered before being triggered:

Plugins should register inside boot modules, named `boot/<plugin_id>`. The
trigger module depends on a new `elgg/booted` module, which loads and
initializes all plugin boot modules in priority order, then triggers
the system hooks.

Plugin boot modules return an instance of `elgg/Plugin`.

Formally deprecates the `boot, system` hook.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 1, 2015

feature(javascript): moves hooks to dedicated AMD modules, adds plugi…
…n boot modules

Deprecates `elgg.register_hook_handler()` and `elgg.trigger_hook()` in favor
of dedicated `elgg/hooks/register` and `elgg/hooks/trigger` modules. These and
other new modules ensure all hooks are registered before being triggered:

Plugins should register inside boot modules, named `boot/<plugin_id>`. The
trigger module depends on a new `elgg/booted` module, which loads and
initializes all plugin boot modules in priority order, then triggers
the system hooks.

Plugin boot modules return an instance of `elgg/Plugin`.

Formally deprecates the `boot, system` hook.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 1, 2015

feature(javascript): moves hooks to dedicated AMD modules, adds plugi…
…n boot modules

Deprecates `elgg.register_hook_handler()` and `elgg.trigger_hook()` in favor
of dedicated `elgg/hooks/register` and `elgg/hooks/trigger` modules. These and
other new modules ensure all hooks are registered before being triggered:

Plugins should register inside boot modules, named `boot/<plugin_id>`. The
trigger module depends on a new `elgg/booted` module, which loads and
initializes all plugin boot modules in priority order, then triggers
the system hooks.

Plugin boot modules return an instance of `elgg/Plugin`.

Formally deprecates the `boot, system` hook.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 1, 2015

feature(javascript): moves hooks to dedicated AMD modules, adds plugi…
…n boot modules

Deprecates `elgg.register_hook_handler()` and `elgg.trigger_hook()` in favor
of dedicated `elgg/hooks/register` and `elgg/hooks/trigger` modules. These and
other new modules ensure all hooks are registered before being triggered:

Plugins should register inside boot modules, named `boot/<plugin_id>`. The
trigger module depends on a new `elgg/booted` module, which loads and
initializes all plugin boot modules in priority order, then triggers
the system hooks.

Plugin boot modules return an instance of `elgg/Plugin`.

Formally deprecates the `boot, system` hook.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 1, 2015

feature(javascript): moves hooks to dedicated AMD modules, adds plugi…
…n boot modules

Deprecates `elgg.register_hook_handler()` and `elgg.trigger_hook()` in favor
of dedicated `elgg/hooks/register` and `elgg/hooks/trigger` modules. These and
other new modules ensure all hooks are registered before being triggered:

Plugins should register inside boot modules, named `boot/<plugin_id>`. The
trigger module depends on a new `elgg/booted` module, which loads and
initializes all plugin boot modules in priority order, then triggers
the system hooks.

Plugin boot modules return an instance of `elgg/Plugin`.

Formally deprecates the `boot, system` hook.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 1, 2015

feature(javascript): moves hooks to dedicated AMD modules, adds plugi…
…n boot modules

Deprecates `elgg.register_hook_handler()` and `elgg.trigger_hook()` in favor
of dedicated `elgg/hooks/register` and `elgg/hooks/trigger` modules. These and
other new modules ensure all hooks are registered before being triggered:

Plugins should register inside boot modules, named `boot/<plugin_id>`. The
trigger module depends on a new `elgg/booted` module, which loads and
initializes all plugin boot modules in priority order, then triggers
the system hooks.

Plugin boot modules return an instance of `elgg/Plugin`.

Formally deprecates the `boot, system` hook.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 2, 2015

feature(js): adds plugin boot modules and modules based on system events
To reduce race conditions, plugins can create boot modules, named like
`boot/<plugin_id>`. All these modules are loaded before the `init, system`
hook.

Depending on the new `elgg/init` module ensures your code runs after this
process.

Similarly, you can depend on the new `elgg/ready` module to ensure all ready
hooks have been called.

Plugin boot modules return an instance of `elgg/Plugin`.

Fixes #7926
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 2, 2015

Member

New PR #8996. Much simpler. Sorry, went crazy with #8993.

Member

mrclay commented Oct 2, 2015

New PR #8996. Much simpler. Sorry, went crazy with #8993.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Oct 2, 2015

feature(js): adds plugin boot modules and modules based on system events
To reduce race conditions, plugins can create boot modules, named like
`boot/<plugin_id>`. All these modules are loaded before the `init, system`
hook.

Depending on the new `elgg/init` module ensures your code runs after this
process.

Similarly, you can depend on the new `elgg/ready` module to ensure all ready
hooks have been called.

Plugin boot modules return an instance of `elgg/Plugin`.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Dec 7, 2015

feature(js): adds plugin boot modules and modules based on system events
To reduce race conditions, plugins can create boot modules, named like
`boot/<plugin_id>`. All these modules are loaded before the `init, system`
hook.

Depending on the new `elgg/init` module ensures your code runs after this
process.

Similarly, you can depend on the new `elgg/ready` module to ensure all ready
hooks have been called.

Plugin boot modules return an instance of `elgg/Plugin`.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Dec 7, 2015

feature(js): adds plugin boot modules and modules based on system events
To reduce race conditions, plugins can create boot modules, named like
`boot/<plugin_id>`. All these modules are loaded before the `init, system`
hook. Registering for plugin hooks should be done inside these.

Depending on the new `elgg/init` module ensures your code runs after this
process.

Similarly, you can depend on the new `elgg/ready` module to ensure all ready
hooks have been called.

Plugin boot modules return an instance of `elgg/Plugin`.

A new hook `config, ckeditor` now filters the CKEditor config object, and
plugins can reliably register for it in a boot module.

Fixes #7926

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Dec 7, 2015

feature(js): adds plugin boot modules and modules based on system events
To reduce race conditions, plugins can create boot modules, named like
`boot/<plugin_id>`. All these modules are loaded before the `init, system`
hook. Registering for plugin hooks should be done inside these.

Depending on the new `elgg/init` module ensures your code runs after this
process.

Similarly, you can depend on the new `elgg/ready` module to ensure all ready
hooks have been called.

Plugin boot modules return an instance of `elgg/Plugin`.

A new hook `config, ckeditor` now filters the CKEditor config object, and
plugins can reliably register for it in a boot module.

Fixes #7131
Fixes #7926

@mrclay mrclay closed this in #8996 Dec 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment