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

Use of `defer()` leads to unresolvable dependency #9380

Open
johnspackman opened this Issue Aug 19, 2017 · 1 comment

Comments

Projects
None yet
1 participant
@johnspackman
Member

johnspackman commented Aug 19, 2017

TL;DR: the defer method for classes is being abused and is causing side effects which could potentially cause the generator or qooxdoo-compiler to break unpredictably; this is easily solved by making minor changes to environment checks and adding a startup event handler.

The defer method provides a means to do some kind of initialisation sometime after the class has been fully defined; this deliberately includes the ability to modify the classes definition, and to that end it is even passed a method which can be used to add properties to the class dynamically.

Over time the defer() method has come to be used as a general purpose initialisation hook. However this causes an unsolvable problem with calculating correct load order for scripts - because the defer method is called as the last step in qx.Class.define, then all the classes which that function refer to have to have been loaded and had their defer method called already. If you call a method in another class from your defer method, then all of the classes which it uses have to be loaded (and had their defer method called), as do any classes which those classes refer to etc etc. It is not difficult to create recursive dependencies - and in fact there already are recursive dependencies in Qooxdoo.

BTW - recursive dependencies are usually fine, so long as the class definition is loaded - defer causes a problem because arbitrary code can be run before some classes can be loaded. This means that defer has side effects which are very difficult (or impossible?) to predict.

AIUI the generator solves this (although not completely) by doing a deep, stateful inspection of the code - i.e. it reads the code and tries to predict what the execution path would be at runtime to see what the load order of classes needs to be. However, this is not perfect and compiler hints (@ignore, @use, and @require) have to be used judiciously to tweak the load order when the generator cannot determine accurately enough. In the future, this means that a perfectly legitimate change to one function in one class, could break the load order because another, completely unrelated class happens to use it. Worse, that breakage might not be reproducable in a sample application because a sample application would have different dependencies to the real-world problem.

qooxdoo-compiler does not take this approach, largely because that kind of deep stateful inspection would be a huge overhead of complexity and anyway would not stop the load order being fragile and susceptible to breaks. By delaying the defer method until it's actually needed, it's been possible to unwind many of the recursive dependencies in the load order, but that is also not a perfect solution.

Looking at the code, the problem stems from using defer() for purposes other than it was originally intended, ie for performing general purpose initialisation, and if we can provide alternative mechanisms for that initialisation the problem is easily solvable.

The defer methods in Qooxdoo classes perform these tasks:

  • Environment - many classes (especially those under qx.bom.*) use defer purely to register implementation methods so that things like qx.core.Environment.get("html.canvas") work
  • Registration - classes under qx.event.* register themselves with qx.event.Registration.addDispatcher, classes under qx.module.*register themselves with qxWeb, and a few other classes (eg qx.io.remote.transport.*) do similar things
  • Mixins qx.ui.core.MChildrenHandling.remap() and qx.ui.core.MLayoutHandling.remap()
  • Mixins again use qx.Class.include to control the mixing in of mixins
  • qx.ui.core.queue.Manager replaces qx.html.Element._scheduleFlush (!!!)
  • Add shutdown event handler
  • initialise static variables

These boil down to three distinct requirements:

  1. Environment - we need a way to identify that a function performs a certain environment test;
  2. Post load initialisation / registration / etc - AFAICT there is no reason that the registration methods need to called as part of class initialisation - they only need to be called once classes are loaded, and before the application itself starts up. The same is true for adding shutdown hooks etc.
  3. Genuine need for defer() - the remaining uses are for legitimate needs, eg they initialise static members that cannot be done as part of the class declaration, and they do not have side effects. Defer needs to remain, but a warning should be output by the compiler when references are made to outside the class (qooxdoo-compiler already does output warnings about this)

Implementation

Environment

As Environment detection functions are standalone, static methods it would be ideal to be able to declare them in the class - eg:

qx.Class.define("qx.bom.client.Browser.getName", {
  extend: qx.core.Object,
  environment: {
    "browser.name": "getName"
  },
  statics: {
    /**
     * Checks for the name of the browser and returns it.
     * @return {String} The name of the current browser.
     * @internal
     */
    getName : function() {
      var agent = navigator.userAgent;
      /* ... snip ... */

This solution will require changes to the generator.py to support it, because it needs to be able to detect that the environment check is provided

Post load initialisation event handler

Again, this could be implemented as a new key:

  /**
   * Called during application startup
   */
  startup: function() {
    qx.event.Registration.addDispatcher(qx.event.dispatch.Direct);
  }

Backwards Compatibility

Because defer is not being removed, existing classes will not break - however, this will require changes to the generator to support the new environment key

@johnspackman

This comment has been minimized.

Show comment
Hide comment
@johnspackman

johnspackman Sep 8, 2017

Member

There is already a mechanism to make environment settings declarative rather than a call in defer - qx.Class.define already supports an environment key, so there is no backwards compatibility issue, just changes to defer()

There also needs to be an optional startup method that is called immediately before application startup.

Member

johnspackman commented Sep 8, 2017

There is already a mechanism to make environment settings declarative rather than a call in defer - qx.Class.define already supports an environment key, so there is no backwards compatibility issue, just changes to defer()

There also needs to be an optional startup method that is called immediately before application startup.

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