Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upHandle case of refreshed cache entry still receiving data #21079
Conversation
highfive
commented
Jun 22, 2018
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jun 22, 2018
|
|
|
r? @jdm |
gterzian
force-pushed the
gterzian:fix_cache_refresh
branch
3 times, most recently
from
87a52ae
to
313506e
gterzian
changed the title
|
I just realized we probably don't want to keep the Looking into it and will update this shortly... |
|
Ok, this seems to do the trick. So the problem is the In the case that the cached resource is actually still receiving data(from another fetch worker), then we set the channel to a new Once the response is 'done', we set the channel back to None... If the cached resource that is being refreshed is already done or empty, we set the channel to None immediately. Ready for review. |
|
@gterzian Is it possible to write a unit test or web platform test for this behaviour? |
| ResponseBody::Receiving(..) => { | ||
| let (done_sender, done_receiver) = channel(); | ||
| *done_chan = Some((done_sender.clone(), done_receiver)); | ||
| cached_resource.awaiting_body.lock().unwrap().push(done_sender); |
This comment has been minimized.
This comment has been minimized.
jdm
Jun 22, 2018
Member
Is there a risk of deadlock here while the cached_resource.body lock is held? Would it be safer to do something like:
let in_progress_channel = match *cached_resource.body.lock().unwrap() {
ResponseBody::Receiving(..) => Some(channel())
ResponseBody::Empty | ResponseBody::Done(..) => None
};
match in_progress_channel {
Some((done_sender, done_receiver)) => {
cached_resource.awaiting_body.lock().unwrap().push(done_sender.clone());
*done_chan = Some((done_sender, done_receiver));
}
None => *done_chan = None,
}
gterzian
force-pushed the
gterzian:fix_cache_refresh
branch
6 times, most recently
from
7feaa36
to
e879ae6
|
@jdm I ended up adding a fairly focused unit-test, mainly because I wasn't able to reproduce the error with either a wpt or a more integration-like fetch test. I'm fairly satisfied because the unit-test did initially fail, and it covers the logical error... |
|
@bors-servo r+ |
|
|
Handle case of refreshed cache entry still receiving data <!-- Please describe your changes on the following line: --> When refreshing cached entries, handle the case the the entry is still "receiving"... --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #20802 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21079) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
|
|
|
|
gterzian commentedJun 22, 2018
•
edited by SimonSapin
When refreshing cached entries, handle the case the the entry is still "receiving"...
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is