Freeze coercers at construction and synchronize Grape::Util::Cache#2819
Open
ericproulx wants to merge 1 commit into
Open
Freeze coercers at construction and synchronize Grape::Util::Cache#2819ericproulx wants to merge 1 commit into
ericproulx wants to merge 1 commit into
Conversation
ericproulx
force-pushed
the
freeze-coercers-and-sync-caches
branch
from
July 25, 2026 19:31
d35752e to
9205570
Compare
Danger ReportNo issues found. |
Extends the validator freeze contract to the whole coercion layer, naming the mechanism as Grape::Util::FreezeOnNew — extend into a class whose instances are shared across requests and new returns a frozen instance. A future request-time lazy ivar write raises FrozenError in the first spec run instead of being a latent race. The module wraps new, never initialize, so the freeze lands after the entire initialize chain (SetCoercer assigns ivars after super); extending a hierarchy base covers its subclasses via singleton-class inheritance. - Extended into DryTypeCoercer (covers Primitive/Array/Set), CustomTypeCoercer (+ collection subclass), MultipleTypeCoercer (internal @type_coercers also frozen) and VariantCollectionCoercer. Validators::Base's hand-written new/super.freeze and ContractScopeValidator's end-of-initialize freeze now use the same module. DeepFreeze's obsolete "coercers memoize lazily" note updated. - Grape::Util::Cache lookups are Monitor-synchronized. The caches are written at API *definition* time (params blocks run at class-body evaluation), which the compile-time Instance::LOCK does not cover and which can be concurrent (parallel eager loading, concurrent autoload of different API files). Monitor rather than Mutex because CoercerCache re-enters itself when a multiple-type coercer builds member coercers. - Route#regexp_capture_index is assigned eagerly in #to_regexp instead of memoized on first read: the reader runs during request-time route matching on shared instances. Previously safe only because to_regexp happened to warm it during compilation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ericproulx
force-pushed
the
freeze-coercers-and-sync-caches
branch
from
July 25, 2026 19:50
9205570 to
214be68
Compare
This was referenced Jul 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Thread-safety hardening for the coercion layer, following the validation-subsystem audit (#2816/#2817). Three changes, one theme: make shared state provably read-only after construction, and synchronize the one write path that genuinely can run concurrently.
1. Coercers freeze at construction — via a named
Grape::Util::FreezeOnNewmoduleValidators have had this guarantee for a while (
Base.new→super.freeze); coercers — equally shared across requests and additionally cached process-wide inTypes::CoercerCache— did not. The blocker is even documented inDeepFreeze's comment: "Coercers (e.g. ArrayCoercer) — use lazy ivar memoization at request time". #2817 removed that memoization, so the exclusion is obsolete.The idiom now has a name instead of five hand-written copies:
extend Grape::Util::FreezeOnNewis declared on each hierarchy base —DryTypeCoercer(covers Primitive/Array/Set via singleton-class inheritance),CustomTypeCoercer(covers the collection subclass),MultipleTypeCoercer(whose internal@type_coercersarray is also frozen), andVariantCollectionCoercer.Validators::Base's hand-writtennew/super.freezeandContractScopeValidator's end-of-initializefreezeare converted to the same declaration, so a grep forFreezeOnNewnow enumerates every class with the immutability guarantee.The module deliberately wraps
new, neverinitialize: the freeze lands after the full subclass initialize chain —SetCoercerassigns@coercer = nilaftersuper, which an initialize-wrapper would break. The freeze is shallow: user-suppliedcoerce_withprocs and custom type classes are never frozen.Payoff: any future request-time
||=in a coercer becomes a deterministicFrozenErrorin the first spec run instead of a latent JRuby race. The full suite passing with every coercer frozen is itself the proof that no hidden mutation remains.2.
Grape::Util::Cachelookups are Monitor-synchronizedThe compile-time
Instance::LOCKserializes everything written duringInstance.new— but the validation caches (CoercerCache,DryTypes::ParamsCache/StrictCache) are written at API definition time: aparamsblock runs when the class body is evaluated, outside any lock. Two non-exotic scenarios make that concurrent:CoercerCache.On MRI the GVL makes this benign; on JRuby/TruffleRuby (in the edge CI matrix) concurrent unsynchronized
Hashwrites can corrupt or raise. OneMonitorin the base class covers all eight cache subclasses. Monitor rather than Mutex becauseCoercerCachere-enters itself: building aMultipleTypeCoercercallsTypes.build_coercerper member type, which lands back in the same cache — a non-reentrant lock would deadlock (there's a spec pinning exactly this path). Overhead is an uncontended lock on boot-frequency lookups; the caches are never read on the hot request path.3.
Route#regexp_capture_indexassigned eagerlybase_route.rbhad@regexp_capture_index ||= CaptureIndexCache[@index]— a lazy memo on sharedRouteinstances reached from request-time route matching (router.rboptimized/greedy match). It was safe only by side effect:to_regexphappened to call it during router compilation, warming it before any request. Same fragile shape as the pre-#2817ArrayCoercer. The index is now assigned directly into_regexp(where@indexwas already being stored — that ivar is gone), the reader is a plainattr_reader, and the request path is provably read-only. This also clears the way for freezingRouteinstances entirely later.Tests
types_spec.rb: every coercer shape (Integer,Array[Integer], nestedArray[Array[Integer]],Set[Integer],JSON, multiple[Integer, String],coerce_withmethod, custom type, directVariantCollectionCoercer) returns frozen frombuild_coercer, and a frozen multiple-type coercer still coerces.spec/grape/util/cache_spec.rb: 8 threads racing a cold key compute it exactly once with consistent results; the reentrantCoercerCachepath completes (deadlock regression guard).route_spec.rb:regexp_capture_indexreadable on a frozen route afterto_regexp— proof no ivar is written after compilation.spec/grape/util/freeze_on_new_spec.rb: pins the module's two load-bearing properties — subclass coverage via singleton-class inheritance, and freezing only after the whole initialize chain (subclasses may assign ivars aftersuper).Full suite: 2385 examples, 0 failures. RuboCop clean.
🤖 Generated with Claude Code