fix(project-cache): Merge instead of replace project state channel in upstream #3952
Conversation
Dav1dde
force-pushed
the
dav1d/upstream-project-merge-channel
branch
from
2e3bc6b
to
baa1864
Compare
|
|
||
| self.other.push(channel); | ||
| self.other.extend(other); | ||
| self.deadline = self.deadline.max(deadline); |
There was a problem hiding this comment.
Should this be the minimum? In theory the deadline could get bumped indefinitely here, which maybe isn't a bad thing?
There was a problem hiding this comment.
I think max is correct, matches the previous behavior more closely.
There was a problem hiding this comment.
I think min matches the previous behaviour more closely since we'd replace the newer entry with the older entry and the deadline is only computed once.
| channel: BroadcastChannel<ProjectFetchState>, | ||
| // Additional broadcast channels tracked from merge operations. | ||
| other: Vec<BroadcastChannel<ProjectFetchState>>, |
There was a problem hiding this comment.
nit: Could replace channel, other by a single SmallVec, to clarify that there is no real difference between the channels.
There was a problem hiding this comment.
I explicitly have separate fields for this to guarantee there is always at least one channel, otherwise attaching requires a dance of channels.last().is_some().or_insert().
|
|
||
| self.other.push(channel); | ||
| self.other.extend(other); | ||
| self.deadline = self.deadline.max(deadline); |
There was a problem hiding this comment.
I think max is correct, matches the previous behavior more closely.
Agreed just need to think of how to actually test this :( |
|
Updated and added a test for exactly the case which was broken. |
Also described in code, while there is a request in flight we might get another request for the same project, when the response for the project is pending we replace the channel instead of merging it.
#skip-changelog