-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Allow CombinedContentSource to need data without locking up #3124
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
🟢 CI successful 🟢Thanks |
Benchmark for 2ca490c
Click to view full benchmark
|
|
||
/// The paused state (partially processed path, content source, vary data) | ||
/// of the internal content source which asked for vary data. | ||
pending: Option<NeededData>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Since we don't actually use pending.vary
, this should be its own PausableCombinedContentSourcePendingState
struct with only a path
and a source
.
That way, you would not need the fiddling below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Benchmark for afc9836Click to view benchmark
|
Benchmark for a9d1ec0
Click to view full benchmark
|
…urbo#3124) Currently when an individual `ContentSource` inside a `CombinedContentSource` requests additional vary data, the combined source will treat that source as the winner of the query. But, sometimes the vary data is necessary to determine that the content is not found by this source. And if that's the case, we'll hit a 404 when we should actually keep iterating. This is exactly the case we're going to hit with redirect/rewrites. Redirects need to be the very first content source, and I also need query/header/cookie/hostname data in order to determine if a redirect applies. Without this, **every** request becomes a 404.
…urbo#3124) Currently when an individual `ContentSource` inside a `CombinedContentSource` requests additional vary data, the combined source will treat that source as the winner of the query. But, sometimes the vary data is necessary to determine that the content is not found by this source. And if that's the case, we'll hit a 404 when we should actually keep iterating. This is exactly the case we're going to hit with redirect/rewrites. Redirects need to be the very first content source, and I also need query/header/cookie/hostname data in order to determine if a redirect applies. Without this, **every** request becomes a 404.
Currently when an individual
ContentSource
inside aCombinedContentSource
requests additional vary data, the combined source will treat that source as the winner of the query. But, sometimes the vary data is necessary to determine that the content is not found by this source. And if that's the case, we'll hit a 404 when we should actually keep iterating.This is exactly the case we're going to hit with redirect/rewrites. Redirects need to be the very first content source, and I also need query/header/cookie/hostname data in order to determine if a redirect applies. Without this, every request becomes a 404.