Fixing hidden infinite loop in the build pending and attach cycle #3355
Conversation
| for (let i = 0; i < this.pendingBuildResources_.length; i++) { | ||
| const resource = this.pendingBuildResources_[i]; | ||
| if (this.documentReady_ || hasNextNodeInDocumentOrder(resource.element)) { | ||
| this.isCurrentlyBuildingPendingResources_ = true; |
There was a problem hiding this comment.
Do this outside the loop. 😛
| if (this.isCurrentlyBuildingPendingResources_) { | ||
| return; | ||
| } | ||
| this.isCurrentlyBuildingPendingResources_ = true; |
There was a problem hiding this comment.
Wrap the following in try {…} finally {}. Otherwise an exception can make this stuck forever.
There was a problem hiding this comment.
Also test these states in some of the existing tests and maybe throw one in that throws an exception.
There was a problem hiding this comment.
That sounds good. Does this mean if one resource keeps failing to build we're going to be stuck and not able to build all other pending resources?
Should we also catch a failed build and remove resource from pending resources?
There was a problem hiding this comment.
With the try-finally we should not get stuck. Maybe I misunderstand the question.
There was a problem hiding this comment.
The try-finally you were suggesting was for the the full for-loop right? But if the resource.build() step keeps failing we're never taking the resource from the this.pendingBuildResources_ array. So next time the loop is going to run same thing going to happen, no?
There was a problem hiding this comment.
I see. Make it so that doesn't happen :)
There was a problem hiding this comment.
Done. Had to break the method into multiple methods to make it a bit more readable.
Let me know what do you think and if I should add some more test cases.
|
PTAL |
| if (this.documentReady_ || hasNextNodeInDocumentOrder(resource.element)) { | ||
| try { | ||
| resource.build(); | ||
| } finally { |
There was a problem hiding this comment.
Why the finally? This would still throw.
There was a problem hiding this comment.
Dropped. I seem to have been very sleepy yesterday... Will revert back some of these changes.
|
Thanks for the reviews. PTAL |
| if (this.documentReady_ || | ||
| hasNextNodeInDocumentOrder(resource.element)) { | ||
| // Remove resource before build to remove it from the pending list | ||
| // in either case the build succeeed or throws an error. |
There was a problem hiding this comment.
Thinking about it, is it possible for #build to throw? We have a try-catch already.
There was a problem hiding this comment.
Yeah you're right. I kinda skipped this resource.build method and was looking at the element.build.
This might just to be safe not to get stuck in that state of deadlocking. But @cramforce what do you think?
There was a problem hiding this comment.
Slightly confused. I think the CL is fine like it is :)
There was a problem hiding this comment.
Yes, we do try/catch already. But if it's critical for this loop - we can add another one.
There was a problem hiding this comment.
#3355 (comment) is unnecessary because the loop can never throw.
There was a problem hiding this comment.
I am cool with having another try-catch, but I agree with @jridgewell that might not be necessary. Let me know if you guys feel strongly about adding it back.
|
PTAL |
| @@ -392,7 +395,7 @@ export class Resources { | |||
| // Build resource immediately, the document has already been parsed. | |||
| resource.build(); | |||
| this.schedulePass(); | |||
| } else { | |||
| } else if (!resource.element.isBuilt()) { | |||
There was a problem hiding this comment.
We already have the element variable.
|
PTAL |
|
LGTM. |
|
LGTM |
|
LGTM |
Fixes #3354
Tested the test case added before and after the test and it does catches the problem before the change (by disconnecting karma) and passes afterwards.
cc/ @jridgewell @erwinmombay
💯 Thanks to @jridgewell for the awesome quick debugging and the solution proposals in #3354!