fix: coroutine safe markdown rendering#386
Merged
Merged
Conversation
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.
This PR makes Markdown mail rendering safe under concurrent Swoole coroutines. Markdown mail rendering previously changed shared worker state while rendering:
EncodedHtmlString::encodeUsing()was temporarily changed and then flushed.mailnamespace replaced between HTML and text renders.Markdownrenderer.That's fine for per-request PHP but unsafe in Hypervel where multiple coroutines can render mail at the same time. Under concurrent HTML and text renders,
mail::layout,mail::message, and related components could resolve through the wrong path mid-render, mixing HTML and text mail components.There was also a separate single-render bug: a Markdown render could clear a boot-time
EncodedHtmlString::encodeUsing()encoder even when secured markdown encoding was not active.Changes
EncodedHtmlString::withEncoding()for coroutine-local temporary encoder overrides.Factory::__clone()so Markdown can render through an isolated view factory/finder clone.mailnamespace only on the cloned factory.<x-mail::...>component compilation to resolve through the current$__env, so mail components stay on the cloned factory. This is a deliberate Laravel divergence; upstream resolves the singleton view factory through the container.MailMessage::render()to use the per-render theme path. ThemarkdownRenderer()helpers were removed because they only existed to mutate the singleton.<x-mail::...>resolution, and theme isolation.Normal view rendering remains on the original fast path. The temporary
mail::namespace swap is now local to just Markdown rendering and does not add overhead to regular package views.Alternative Approaches
My first iteration put temporary view namespace overrides in
CoroutineContext. That fixed the race, but every normal namespaced view lookup (package::view,pagination::default, etc.) had to check for an active override. In package-heavy apps, that adds a lot of unnecessary context lookups to the global view path for a Markdown-specific problem.I also considered registering separate internal namespaces for HTML and text mail components. That keeps the finder hot path clean, but it changes template semantics and misses dynamic component names like
$component = 'mail::message'.The clone approach keeps the normal view finder unchanged, preserves
mail::semantics, supports dynamic names, and pays the small isolation cost only when Markdown mail is actually rendered.