fix: avoid caching manifests as promises #7468
Conversation
|
License in lockfiles appears to be something that is not well documented, either if we support it or not. I recently closed #7384 because I tested back to npm 8 and we did not appear to be adding license to the lockfile. I don't know what purpose it serves there, it does not have anything to do w/ reification. We'd no more want the license in there than the |
|
I think I know why we were caching the promise and not the manifest. The idea was that we wouldn't block the event loop until the manifest was actually awaited. I don't know if this was a useful optimization. This could be the source of our race condition between links and manifests. One of the things we do currently (before this PR) is queue up these manifest requests, move on to add items to What I do know is we should take a glance at the places where we were relying on this deferment and see if we still want to do keep deferring. |
Somewhat related to #7276 and #7463. I don't think there is a reason to cache the promise here. And if we ever did choose to replace this with an LRUCache we would need to know the size of what we are caching which will be easier if we only cache the resulting manifest. I also added a comment about why I think we are removing the license from manifests here. License was removed in #7126 which looks to be a purposeful change but I could not find a reason. Adding the license back in causes many snapshots to fail because the license is now present in lockfiles, so that's how I came up with the comment.
lukekarrys
force-pushed
the
lk/orange-future-noon
branch
from
8913a18
to
49d044f
Compare
|
I'm going to close this as we aren't currently using an LRUCache here so this is a premature optimization. If we ever did this in the future, we could make this or similar changes. I'm hesitant to make any "if its not broken, don't fix it" changes. And I want to link this to #7411 since the disabling of this test was due to another time we changes caching promises to only caching the result. |
Originally this was in #7468: We backed off of it while we were rebuilding pacote's packument cache. Now that that's done we can assess this in isolation. I think it makes sense. The packument is cached here, all this is awaiting is normalization and ssri calculation. The only place this potentially does anything is in the premature loading of manifests in `#buildDepStep` when looking at problem edges. We can just wait till we need them and not throw a ton of requests in parallel before we actually need them. Removing the premature loading in problem edges will have to be a separate effort, as it is somehow load bearing
Originally this was in #7468: We backed off of it while we were rebuilding pacote's packument cache. Now that that's done we can assess this in isolation. I think it makes sense. The packument is cached here, all this is awaiting is normalization and ssri calculation. The only place this potentially does anything is in the premature loading of manifests in `#buildDepStep` when looking at problem edges. We can just wait till we need them and not throw a ton of requests in parallel before we actually need them. Removing the premature loading in problem edges will have to be a separate effort, as it is somehow load bearing
Somewhat related to #7276 and #7463. I don't think there is a reason to cache the promise here. And if we ever did choose to replace this with an LRUCache we would need to know the size of what we are caching which will be easier if we only cache the resulting manifest.
I also added a comment about why I think we are removing the license from manifests here. License was removed in #7126 which looks to be a purposeful change but I could not find a reason. Adding the license back in causes many snapshots to fail because the license is now present in lockfiles, so that's how I came up with the comment.Edit: opened #7475 to address the license/lockfile behavior.