Make module script more spec-compliant so that no need to do a cyclic checking #26903
Labels
A-content/script
Related to the script thread
Comments
4 tasks
This was referenced Jun 22, 2020
CYBAI
added a commit
to CYBAI/servo
that referenced
this issue
Jun 22, 2020
In servo#26395, we moved from `recursive checking` of dependency status to check only the _current module_'s dependency status and its descendant dependency status and also circular dependency status. However, it will cause an issue. For example, if the module dependency is like following ``` a -> b -> c -> d -> e f -> g -> h -> c -> d -> e ``` In this example, if the d module is still under fetching but g is trying to advance to finish. Then, it will cause a panic because module d is g's grand-grand-grand-descendant which means it's still under fetching and we can't instantiate module g. Ideally, we should get rid of the checking in servo#26903 so, before servo#26903 fixed, we can just move back to the recursive checking way which will ensure all descendants are not fetching.
CYBAI
added a commit
to CYBAI/servo
that referenced
this issue
Jun 23, 2020
In https://github.com/servo/servo/pull/26395/files#diff-3fe97584f564214ec8e7ebbf91747e03L253-R318, we moved from `recursive checking` of dependency status to check only the _current module_'s dependency status and its descendant dependency status and also circular dependency status. However, it will cause an issue. For example, if the module dependency is like following ``` a -> b -> c -> d -> e f -> g -> h -> c -> d -> e ``` In this example, if the d module is still under fetching but g is trying to advance to finish. Then, it will cause a panic because module d is g's grand-grand-grand-descendant which means it's still under fetching and we can't instantiate module g. Ideally, we should get rid of the checking in servo#26903 so, before servo#26903 fixed, we can just move back to the recursive checking way which will ensure all descendants are not fetching.
bors-servo
added a commit
that referenced
this issue
Jun 23, 2020
Fix nested modules while imported under more than 3 levels This is kind of workaround to fix the issue but #26903 should provide much better solution to remove the checking. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #27029 - [x] There are tests for these changes <!-- 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. -->
bors-servo
added a commit
that referenced
this issue
Jun 23, 2020
Fix nested modules while imported under more than 3 levels This is kind of workaround to fix the issue but #26903 should provide much better solution to remove the checking. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #27029 - [x] There are tests for these changes <!-- 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. -->
bors-servo
added a commit
that referenced
this issue
Jun 23, 2020
Fix nested modules while imported under more than 3 levels This is kind of workaround to fix the issue but #26903 should provide much better solution to remove the checking. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #27029 - [x] There are tests for these changes <!-- 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. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Follow-up of #26395:
In the refactoring in #26395 to move away from the
Promise.allway, I've noticed we still need to do a checking to see if the to-be-finished module has any circular modules under fetching or not (code at here).However, in both Gecko and Blink, both of them don't need to do that check at all. Even I've tried to imitate Blink to do the
incomplete_urlschecking, we still need another check for cyclic dependencies. @Manishearth pointed out that this might mean we might actually not be spec-compliant. (#26395 (comment))So, with the improvement in #26395, we've removed many redundant checking; however, we will want to make it even better and more spec-compliant so that we don't need to check if there's any under fetching modules when we're going to finish a module.
(I don't figure out why current implementation is not spec-compliant yet.)
The text was updated successfully, but these errors were encountered: