-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Execution paths missing @context #1205
Comments
Having some discussions, it looks like a possible avenue is to use a thread local instead of trying to manually set the 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 Using a local thread would allow us to completely avoid the development gymnastic of linking context. We would be able to completely 🔥 the I'm not a fan of global state, but |
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...
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 agree that makes sense. |
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 ( 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 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 |
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 For cases where a drop renders another template, as long as 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 |
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. |
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
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 |
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. |
That makes sense. We just need to have a transition plan to reduce how disruptive the change is.
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 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 |
Actually, |
One of the primary reasons why having 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 The stricter we can make the liquid library, the better we can leverage that for correctness and runtime performance. |
Perhaps if the added strictness deprecation warnings are too disruptive, then we could initially make that opt-in by using something like |
General
I've been struggling for some time with certain code paths in my application where the
@context
withinLiquid::Drop
isnil
and finally decided to deep dive.In liquid, when the original object is retrieved from the vm environment, context is applied properly:
liquid/lib/liquid/variable_lookup.rb
Lines 67 to 68 in 57c9cf6
liquid/lib/liquid/context.rb
Lines 191 to 192 in 57c9cf6
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
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 performto_liquid
and assignation of @context to all the elements. This leaves "non liquid" object when the contract in my opinion should have been thatto_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 theobj.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 doingto_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 callingto_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 ofLiquid::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. Removingcontext=
fromLiquid::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 viato_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.
The text was updated successfully, but these errors were encountered: