Skip to content
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

Fixes for serialization #1380

Merged
merged 2 commits into from
Jan 5, 2021
Merged

Fixes for serialization #1380

merged 2 commits into from
Jan 5, 2021

Conversation

peterzhu2118
Copy link
Member

Commit 1: Refactors setting options to a separate method in Liquid::Template which is reused for deserialization.

Commit 2: Freeze blocks in reverse order of creation. Serialization requires the blocks are frozen in reverse order from when it's created (because it requires that the child is frozen before the parent is frozen).

@dylanahsmith
Copy link
Contributor

Commit 2: Freeze blocks in reverse order of creation. Serialization requires the blocks are frozen in reverse order from when it's created (because it requires that the child is frozen before the parent is frozen).

We should try to avoid relying on the correctness of ruby code to avoid crashes (or C assertion failures which will be crashes outside of development). If I revert this commit and test against Shopify/liquid-c#138, then I get the following error in development (or a crash with DEBUG=false)

Assertion failed: (tag_markup->block_body->compiled), function document_body_write_tag_markup, file ../../../../ext/liquid_c/document_body.c, line 96.
rake aborted!

@peterzhu2118
Copy link
Member Author

Yeah, the problem is that the freeze method is also responsible for writing the blocks to the buffer, and the blocks right now are required to be written in reverse order of creation (so the child is written before the parent because the parent needs to know the offset in order to reference the child). The assertion is there to ensure that this condition is met.

It might be possible to recursively freeze unfrozen children in liquid-c before the parent itself is frozen. But I think that's additional complexity that doesn't need to be there.

@dylanahsmith
Copy link
Contributor

That is a debug assertion though. We could raise instead

@peterzhu2118
Copy link
Member Author

I changed it to raise an error instead of an assertion error in Shopify/liquid-c@61e88b3. The error looks like this:

RuntimeError: child #<Liquid::C::BlockBody:0x00007fdb108278f8> has not been frozen before the parent

@peterzhu2118 peterzhu2118 merged commit abfab3b into master Jan 5, 2021
@peterzhu2118 peterzhu2118 deleted the pz-serialize-compat branch January 5, 2021 19:10
@macournoyer macournoyer temporarily deployed to rubygems January 6, 2021 15:19 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants