Make parser cache opt-in

[gmcgibbon]

👋 I noticed recently that this gem uses the bootsnap iseq cache dir as a basis to enable caching or not. This doesn't make much sense to me because the parser cache has nothing to do with bootsnap. This was causing problems for me on an app that precompiled bootsnap cache on CI, and marked the cache as readonly. Naturally, this caused problems when the GraphQL parser cache got written to.

Since we can't precompile this cache, and it isn't dependent on bootsnap, I think we can simply use a different directory and avoid these conditionals. This will prevent bootsnap cache to be polluted/extended by the graphql gem, and allow more apps to leverage GraphQL caching. WDYT?

[rmosolgo]

This makes sense to me -- @casperisfine, does this seem like an appropriate change to you? (IIRC you added the parser cache, just want to double-check.)

[casperisfine]

So the reasoning was that Bootsnap has similar cache expiration concerns so we basically piggy backed on Bootsnap to get our cache pruned the same way etc, #3156

Using a distinct directory is "fine", but entries might pile up in there, and will need to be pruned regularly otherwise it may cause issues in production.

So while it can be documented it's a bit of a footgun.

[gmcgibbon]

and will need to be pruned regularly

Are we able to build that pruning functionality into this gem for the GQL cache specifically? There's no peer dependency between bootsnap and this gem, so it isn't clear why we need to lean on bootsnap. The PR you linked also doesn't mention what specifically we're depending on in bootsnap's iseq cache logic to prune.

If I were to guess, the marshalling in the cache is what would cause issues, so maybe we can add a task or something to clear it manually, or even leverage a safer cache encoder?

[casperisfine]

The PR you linked also doesn't mention what specifically we're depending on in bootsnap's iseq cache logic to prune.

We depend on that note: https://github.com/Shopify/bootsnap#usage

Note also that bootsnap will never clean up its own cache: this is left up to you. Depending on your deployment strategy, you may need to periodically purge tmp/cache/bootsnap*. If you notice deploys getting progressively slower, this is almost certainly the cause.

I don't mind removing this bootsnap check, but if we do I think it shouldn't be replaced by another directory, it should be entirely opt-in with a clear documentation stating that this directory need to be regularly purged.

[gmcgibbon]

https://github.com/Shopify/bootsnap#usage

Ah, understood. Thanks for clarifying!

it should be entirely opt-in with a clear documentation stating that this directory need to be regularly purged.

Yep, that makes sense, I'll proceed with that approach. 👍

[gmcgibbon]

I wrote a page in the guides that explains this behaviour, so hopefully that helps. I also made the cache opt-in by setting the new config option to false by default.

[rmosolgo]

Thanks for taking another look at this. I hadn't considered the need to purge it.

As written, will it disable the cache for anyone who is currently using it? Given how long this has been here, and how many apps are probably using it, I'd rather not silently disable it.

You mention in the docs how the parser cache can be manually reassigned. Would that work in your app to choose a custom directory? If so, maybe we could leave the default cache setup the way it is, but improve the documentation as you've suggested here.

[gmcgibbon]

As written, will it disable the cache for anyone who is currently using it?

Yes, it will. I understand your concern. What we can do is use the same logic it acted on to enable the cache as the default value (eg. config.graphql.parser_cache = ::Object.const_defined?("Bootsnap::CompileCache::ISeq") && Bootsnap::CompileCache::ISeq.cache_dir).

You mention in the docs how the parser cache can be manually reassigned. Would that work in your app to choose a custom directory?

For my use case, I want to disable it entirely, but using a different directory does also work (and generates excess build artifact in our CI pipeline, not really a huge deal). Right now, I'm assigning a no-op subclass to GraphQL::Language::Parser.cache= which is a bit of a hack in my opinion. Since we have a railtie already, it seems like a good abstraction to specifically enable or disable the cache.

The larger problem I'm also trying to fix here is removing needless references to bootsnap, because it is entirely unrelated to the parser cache. The logic activates the cache for many, but also doesn't activate the cache for others that could benefit, but don't use bootsnap. Because of this, I would strongly suggest we remove the current enabling logic after a deprecation cycle, and use simple railtie configuration going forward. However, I'm open to implementing this feature however you see fit! 🙂

[gmcgibbon]

@byroot / @rmosolgo do you agree with this approach?

[byroot]

IMO this is too much.

[gmcgibbon]

I assume you mean the deprecation warning specifically is too much? Do you think the behaviour change would be obvious enough by reading release notes, or do you have a different idea?

[casperisfine]

I mean the whole if. Changelog should be enough. This cache is a nice to have, but I wouldn't call it a breaking change if it's no longer configured by default.

[gmcgibbon]

👍 If @rmosolgo agrees I'll squash commits and we can proceed.

[rmosolgo]

Yeah, this is alright with me. I think it's too bad to loose the default that (apparently?) worked for almost everyone, but at the same time, I don't have a strong opinion about it and I appreciate that it's now better documented 👍

[rmosolgo]

Thanks for your work here!

[benhutton]

Thank you for this! Our app is not a rails app and does not use bootsnap, so we never even knew we were missing out on something! Going to go enable now...

[mroach]

What does the parser cache get used for? Or, what action can I take to try and trigger something to be cached?
I wasn't aware of this option before, and it wouldn't have been working in our release environments since our Docker containers run with a read-only root filesystem except for a specific writeable ephemeral tmp dir. That leads me to believe it fails silently when it can't write to disk?

In my local development on this branch I tried setting this to true and then running queries, dumping the SDL, and I'm not seeing anything being written to disk.

I'm also generally curious what it's used for and the effects it'll have in our different environments. For production, we only allow stored operations to be executed whereas our "edge" environment allows arbitrary queries to develop/test against.

[casperisfine]

See #3156 and #3155

[rmosolgo]

Hey, I took a quick refresher on this feature myself and wrote a small example script:

require "bundler/inline"

gemfile do
  gem "graphql", "2.1.6"
end

class MySchema < GraphQL::Schema
  class Query < GraphQL::Schema::Object
    field :int, Integer
    def int; 5; end
  end

  query(Query)
end

cache_dir = "cache_dir/"
`rm -rf #{cache_dir}`

filename = "query.graphql"
File.write(filename, "{ int }")
doc = GraphQL::Language::Parser.parse_file(filename)
MySchema.execute(document: doc)

pp Dir.glob(cache_dir + "**/*")
# => []

GraphQL::Language::Parser.cache = GraphQL::Language::Cache.new(Pathname.new(cache_dir))

doc = GraphQL::Language::Parser.parse_file(filename)
MySchema.execute(document: doc)
pp Dir.glob(cache_dir + "**/*")
# => ["cache_dir/d29384b492cf962d44094f2b180898c894c4de42935a8c29b96514dea53671a0"]

In short, it only works with Language::Parser.parse_file. (I'm sure this is an accident, but the shortcut GraphQL.parse_file doesn't use it:

graphql-ruby/lib/graphql.rb

Lines 52 to 55 in ec3f500

	def self.parse_file(filename)
	content = File.read(filename)
	default_parser.parse(content, filename: filename)
	end

)

[mroach]

@rmosolgo Thank you! So it looks like this is only used if you're parsing schema files from disk and doesn't get you anything when using the operation store?

[rmosolgo]

That's right -- it's only used when parsing from disk. (It could be queries, not only schema files.)

If you're looking for a cache in front of persisted queries, here's an example of a simple in-memory cache: https://gist.github.com/rmosolgo/9d77fa2742d2734321d41432a9674c45 I'd be happy to refine that example further if you're interested.