-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
fix(service-worker): fix LruList 'remove' bug #22769
Conversation
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.
Good catch! I suspect this (possibly together with #22325 (comment)) would also fix #22218.
Since #22325 seems to be stale, would you mind setting previous
/next
to null
for remove nodes (just in case 😁).
Updated the PR setting node.previous/next to null before deleting. The change in PR #22325 shouldn't be necessary now since it was mostly a bandaid for this bug. |
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.
.pop()
also needs clearing .next/.previous
.
Or maybe we could make .pop()
just call .remove()
and avoid duplication:
pop(): string|null {
const url = this.state.tail;
if (url !== null) {
this.remove(url);
}
return url;
}
this.state.count--; | ||
return true; | ||
} | ||
|
||
// The node is not the head, so it has a previous. It may or may not be the tail. | ||
// If it is not, then it has a next. First, grab the previous node. | ||
const previous = this.state.map[node.previous !] !; | ||
|
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.
😢
@@ -145,14 +145,14 @@ class LruList { | |||
const next = this.state.map[node.next !] !; | |||
next.previous = null; | |||
this.state.head = next.url; | |||
delete this.state.map[url]; |
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.
Can you set node.next
to null
here too?
BTW, could you also add the following at the end of the commit message:
|
a9e92a8
to
751fb2b
Compare
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.
One nit. LGTM otherwise.
@@ -207,6 +194,7 @@ class LruList { | |||
// The next pointer of the node being inserted gets set to the old head, before the head | |||
// pointer is updated to this node. | |||
node.next = this.state.head; | |||
node.previous = null; |
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.
This is redundant here. It is either a new node (so previous
is already null
) or it is remove()
's job to set .previous
to null
.
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.
Hmmm, yeah I guess you're right. I ran across this issue earlier where the head had a previous node, but that must have been before the switch to using 'remove'.
'remove' method not removing url from state.map 'accessed' method not removing 'previous' reference from existing node when it becomes the head Fixes angular#22218 Fixes angular#22768
'remove' method not removing url from state.map 'accessed' method not removing 'previous' reference from existing node when it becomes the head Fixes angular#22218 Fixes angular#22768 PR Close angular#22769
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
remove method not removing url from state.map
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #22768
What is the new behavior?
Does this PR introduce a breaking change?