Execution paths missing @context

[tjoyal]

General

I've been struggling for some time with certain code paths in my application where the @context within Liquid::Drop is nil and finally decided to deep dive.

In liquid, when the original object is retrieved from the vm environment, context is applied properly:

https://github.com/Shopify/liquid/blob/57c9cf64ebc777fe5e92d4408d31a911f087eeb4/lib/liquid/variable_lookup.rb#L67-L68 https://github.com/Shopify/liquid/blob/57c9cf64ebc777fe5e92d4408d31a911f087eeb4/lib/liquid/context.rb#L191-L192

The @context, is in these cases, used as a global variable to alter behaviour based on the current user for which the template is rendered. This is not always great, but it is what it is it purpose.

Example

require 'liquid'

class BaseDrop < Liquid::Drop
  def initialize(var)
    @var = var
  end

  def value
    if @context.registers.fetch(:upcase)
      @var.upcase
    else
      @var
    end
  rescue StandardError => e
    e.to_s
  end
  alias_method :to_s, :value
end

environments = {
  "single" =>  BaseDrop.new("aaa"),
  "array" => [
    BaseDrop.new("aaa"),
    BaseDrop.new("bbb"),
    BaseDrop.new("ccc"),
  ]
}
registers = {
  upcase: true,
}
context = Liquid::Context.new(environments, {}, registers)

template = 'Result: {{ single | json }}'
Liquid::Template.parse(template).render!(context)
#=> "Result: AAA"

template = 'Result: {{ array | json }}'
Liquid::Template.parse(template).render!(context)
#=> "Result: undefined method `registers' for nil:NilClassundefined method `registers' for nil:NilClassundefined method `registers' for nil:NilClass"

Propose mitigation

Now, I couldn't come up with something great to get around this, maybe just few step to make it less painful.

I see this as a multiple issues.

A) lib/liquid/extensions.rb patches Array#to_liquid. It does not iterate to perform to_liquid and assignation of @context to all the elements. This leaves "non liquid" object when the contract in my opinion should have been that to_liquid is expected to do just that.

B) When using filters, they are pure ruby and can do what they want. I do not think there is much we can do here as the purpose of filters is to enable pretty much any implementation. The only thing I think can be done here is to have filters developers to respect the liquid mentality and when interacting with objects beyond their method inputs to carefully consider if they need to invoke to_liquid on them, and if doing so, they need to attach the obj.context= @context on them.

C) It is too easy to miss appending the context= on liquid drops. Lots of places in my code end up doing to_liquid per the recommendation in B so we do not end up with some code path using one representation while others to have another. Still a lot of cases miss that simply calling to_liquid does not carry over the correct context, sometimes leading to unexpected behaviours (exceptions when invoking method on a nil @context).

While I don't have a suggestion for B, for A and C I think something can be done. I'd like to propose a new mandatory argument to the well known to_liquid which would be the @context (obj.to_liquid(@context)). I couldn't find a clear reason why these 2 operations aren't grouped together. I would even go to say that the context should be part of the initializer of Liquid::Drop, ensuring it presence at all time.

If it was to be built from scratch, I think it would be a good idea. Now it is realistic to do such a large change I am uncertain. I really like the ideas of drops to be initialized with all their needs and to have them become immutable.

A more radical proposition could be to no longer have access to the context in Liquid::Drop and use filters to mutate behaviour in all cases instead. That seems it would require a larger breaking change, might not acceptable. It could still be made an opt-in option. Removing context= from Liquid::Drop and asking those that want to maintain the behaviour to add it back in their drop would work. My worry with this proposition is that as long that it is possible, people will be using it as it is such a powerful tool and there is no point in removing it unless there is a better alternative.

If we desire to avoid changing core signatures, I think we could settle for fixing Array#liquid. Does not really make it clear to developers when they convert an object to a drop via to_liquid or simply instantiating a new drop manually that they need to care about assigning the context manually afterward. We would also need to see if we can make the existing code context assignation logic to iterate over iterable objects in general .

I would really like to at least have to_liquid(context) as this would make it clear that it's a parameter that ideally shouldn't be forgotten.

Thoughts? Rocks? Would love some options.

[tjoyal]

Having some discussions, it looks like a possible avenue is to use a thread local instead of trying to manually set the context=.

If there is a desire to keep the context on drops and filters, what we are basically saying is "we want a global state".

Hijacking to_liquid to add a new mandatory argument might not be realistic as there seems to be a desire(which totally make sense) to add to the environment LiquidDrop objects. It would not be possible to add environment variable in the context initializer as these would need to be linked to said context.

Using a local thread would allow us to completely avoid the development gymnastic of linking context. We would be able to completely :fire: the context= logic from all code paths.

I'm not a fan of global state, but @context is one. We just try out best to hide it.

[pushrax]

A thread local is appealing but a basic implementation would preclude Liquid's use within concurrent applications that don't use a thread-per-connection model. Some kind of per-render key would fix this, but that key would have to be propagated somehow...

A) lib/liquid/extensions.rb patches Array#to_liquid. It does not iterate to perform to_liquid and assignation of @context to all the elements. This leaves "non liquid" object when the contract in my opinion should have been that to_liquid is expected to do just that.

B) When using filters, they are pure ruby and can do what they want. I do not think there is much we can do here as the purpose of filters is to enable pretty much any implementation. The only thing I think can be done here is to have filters developers to respect the liquid mentality and when interacting with objects beyond their method inputs to carefully consider if they need to invoke to_liquid on them, and if doing so, they need to attach the obj.context= @context on them.

Array#to_liquid follows the current contract of Liquid. As you mentioned, filters are expected to call to_liquid on values they pull out. This is done by the InputEnumerator#each method in StandardFilters. It's definitely extra mental and runtime overhead.

I would really like to at least have to_liquid(context) as this would make it clear that it's a parameter that ideally shouldn't be forgotten.

I agree that makes sense.

[dylanahsmith]

A thread local is appealing but a basic implementation would preclude Liquid's use within concurrent applications that don't use a thread-per-connection model.

I'm not sure I understand what this has to do with connections. It is just the render context we would be propagating. Since we are already doing that, I won't see how that would be a problem, since Liquid::Template#render doesn't provide a way to switch threads in the middle of rendering. Using a fiber local variable (Thread#[]) also means this could work for applications using fibers for concurrency.

Although using a fiber local wouldn't preclude the use of Liquid within a concurrent application, those applications would need to avoid trying to get the context from the fiber local within another fiber/thread. For instance, if we changed Drop#context to get the context from a fiber local, then the application would need to get the context from the fiber that started rendering and explicitly pass it to the other fiber if it needs access to it. In this way, it does seem like a potentially breaking change.

---

A big advantage to using a fiber local would be that we could avoid having to manage threading through the context in the liquid library. The only use of the context in the standard filters is to set it on drops, so we could remove the context from the strainer object. The only use of the context in Liquid::Drop itself is for liquid_method_missing to support strict_variables, but it looks like that can be handled by changing the implementation of Drop#key? to delegate to self.class.invokable? and special casing what exception is raised in Context#lookup_and_evaluate based on whether obj.is_a?(Liquid::Drop).

The application could then manage any fiber/thread-local state their filters or drops depend on. Again, they can pass that state explicitly into another fiber or thread that a drop or filter method delegates to.

Not threading the context through everything would also mean avoid performance overhead from having to do e.context = @context if e.respond_to?(:context=) on all the drops that might use the context. Even if we decided to set and unset a fiber local variable in Liquid::Template#render around rendering (e.g. to mostly provide backwards compatibility), that would still just be a pretty small fixed overhead compared to threading it through everywhere in the middle of rendering.

[pushrax]

I'm not sure I understand what this has to do with connections. It is just the render context we would be propagating. Since we are already doing that, I won't see how that would be a problem, since Liquid::Template#render doesn't provide a way to switch threads in the middle of rendering. Using a fiber local variable (Thread#[]) also means this could work for applications using fibers for concurrency.

For instance, if we changed Drop#context to get the context from a fiber local, then the application would need to get the context from the fiber that started rendering and explicitly pass it to the other fiber if it needs access to it. In this way, it does seem like a potentially breaking change.

A possible future for minimizing latency could involve returning futures from drops, and allowing concurrent execution of the template. The basic idea wouldn't directly have an issue with using Thread#[] to store the context, but if we ever wanted custom scheduling in which a pool of threads is used to process/fetch data for multiple Liquid templates concurrently, ensuring the right context is present would be tricky. Though, after thinking this through more, and based on your ideas, there should always be a solution, by storing and restoring contexts in the scheduling code.

For cases where a drop renders another template, as long as Template#render ensures it restores the previous context, the right context should be present at the right time. Or maybe Document#render*; I've seen code that manually constructs a Document before.

I suppose the biggest blockers to this are the two breaking changes identified so far, particularly to test code. I think there are many tests that instantiate a drop, set its context, and call a method on it. I suppose we could set Drop#context= to a method that calls through to Thread#[]=? It seems always possible to construct a case that will break though, e.g. create 2 drops and assign them different contexts, then call a method on each. Then there's the possibility Dylan mentioned of existing code that passes Liquid objects between fibers or threads, but I would guess that's much less common than tests that set the context. Maybe these are small enough to be accepted and put into the next major version though.

[dylanahsmith]

A possible future for minimizing latency could involve returning futures from drops, and allowing concurrent execution of the template.

If we want to go down this path, then I think it would be better to not expose the full context to the other thread. We would need to preserve the appearance that the code is being executed in a single-threaded way, so we would need to make sure that any drop or filter method does all the reads and writes to the context eagerly, so that we don't have code reading the wrong value. So that we don't have to handle any input value being a future, we would probably also need to be explicit about the dependencies and effects of drop and filter methods. Basically, if applications want to use a feature like this, then I think they will have to change their drops and filters, although we could probably provide backwards compatibility for synchronous rendering.

[tjoyal]

I suppose the biggest blockers to this are the two breaking changes identified so far, particularly to test code.

I am really not worried about it. Tests will require a major refactor but I think it's a small price to pay for the advantage of not having to code, test and worry about missing context in all the places.

Something we could explore for a transition (or even longterm support?) could be to sunset the direct use of @context directly and to replace it with:

def context
  @context || load_from_thread
end

I think it would (mostly due to the new method name possible clash with existing attributes) compatible with existing code.

A feature flag could stop assigning context= directly and we could decide to sunset completely or not if desired.

[tjoyal]

To re-iterate, I am not worried about implementation details as much as I am about code simplicity(removal of manual developer responsibility) and output accuracy.

If less code and less tests end up increasing accuracy and performance, I'll be very happy.

[dylanahsmith]

I'd like to propose a new mandatory argument to the well known to_liquid which would be the @context (obj.to_liquid(@context)). I couldn't find a clear reason why these 2 operations aren't grouped together. I would even go to say that the context should be part of the initializer of Liquid::Drop, ensuring it presence at all time.

That makes sense. We just need to have a transition plan to reduce how disruptive the change is.

1. Introduce future compatible APIs (minor release)

• Add Liquid::Drop#initializer(context = nil) to support liquid drops that are initialized with a context
• Add (_context = nil) parameter to Liquid defined to_liquid methods where the context isn't needed
• Add a Liquid::Utils.to_liquid(value, context) method that initially calls value = value.to_liquid and value.context = context if value.respond_to?(:context=), which can be called by code using liquid instead of value.to_liquid to make sure the context is set on drops and work in future versions of liquid.

2. Deprecate old usage (minor release)

• Pass the context to to_liquid methods in Liquid::Utils.to_liquid, using a rescue ArgumentError fallback to support to_liquid methods with arity 0 using the old behaviour for calling to_liquid without an argument and trying to assign the context. This fallback path can also do the deprecation warning.
• Change methods that take an optional context argument to instead take Liquid::Utils.missing_context as the default value, which can then do the deprecation warning and return nil.

3. Remove code on deprecation paths (major release)

• Make context arguments mandatory
• Liquid::Utils.to_liquid(value, context) loses its rescue ArgumentError block
• Updates calls to Liquid::Utils.to_liquid(value, context) to use value.to_liquid(context) directly

The idea would be that libraries could be updated after step 2, but still support the feature release from step 1 as a minimum liquid dependency. They can use Liquid::Utils.to_liquid to keep compatibility with the old and new approach, as well as avoiding the slow fallback path of to_liquid being called with the wrong arity.

Applications could be updated after updating liquid to the feature release from step 2, when they start getting deprecation warnings. This could be done in a single step for small applications, with required context arguments to any to_liquid defined methods, drops being initialized with a context and value.to_liquid(context) being used directly. Although, larger applications can prepare for the upgrade in step 1, using Liquid::Utils.to_liquid for compatibility without performance regressions.

[dylanahsmith]

Actually, Liquid::Utils.to_liquid(value, context) could be shortened to context.to_liquid(value)

[dylanahsmith]

One of the primary reasons why having .to_liquid(context) and initializing drops with the context appeals to me is that it avoids having these values in an invalid state. We could take this further by avoiding having non-liquid values being set in liquid variables, forcing application code or liquid extensions to convert values to a liquid value before setting them. This could even be enforced in a checked mode, which could just be used in development to help catch bugs, which ensures that value.to_liquid(context).equal?(value) and could even do deep checking of builtin collections like Hash and Array.

This would not only help alleviate the concern of filters to handle these nested non-liquid values, it would also avoid any redundant conversions that would otherwise happen when a non-liquid value is accessed and converted multiple times. It would also reduce the performance overhead of having to check for non-liquid values in the hot path of value access.

Another way liquid could be stricter to reduce runtime overhead would be to restrict Proc support to keys on liquid Hash values (unless we want to transition to using liquid drops for those lazy values) and as initial assign/environment values (or potentially separating them into a separate Hash to further reduce .is_a?(Proc) checks).

The stricter we can make the liquid library, the better we can leverage that for correctness and runtime performance.

[dylanahsmith]

Perhaps if the added strictness deprecation warnings are too disruptive, then we could initially make that opt-in by using something like require "liquid/next" and have require "liquid" preserve the current behaviour by wrapping methods that do the current liberal conversions to preserve the current behaviour for longer and allow the performance benefits of the stricter behaviour to be available sooner.