fix: cache parallel moduleInfo calls #17957
Conversation
|
|
| @@ -243,7 +244,19 @@ export class ModuleRunner { | |||
| } | |||
| } | |||
|
|
|||
| private async cachedModule( | |||
| private async cachedModule(url: string, importer?: string) { | |||
| const cacheKey = `${url}~~${importer}` | |||
There was a problem hiding this comment.
Shouldn't the same cached module be returned for different importers if they end up resolving to the same id (that would normally be the case)?
It seems that in getModuleInformation, the cachedModule is keyed only by url. So we may already be resolving to the same cached module for different importers (even if they resolve to different ids) 🤔
There was a problem hiding this comment.
There is no way to guarantee that since we don't have the resolved url. The url here can be ./index.ts - why would you return the same module when it's imported in different places?
There was a problem hiding this comment.
Maybe we can cache it only by url if it starts with / I guess
There was a problem hiding this comment.
I think the cache should be with a resolved id somehow. At transformRequest() we have a similar cache by url, and it works because these are already resolved ids during import analysis (turned into URLs). It could still happen there if someone adds an extra query that is later ignored, the cache will not catch it. It works in that case because people don't do that. It seems the relative urls like ./index.ts are against the root and not the importer (that confused me, and I thought the urlToIdMap would end broken). But the url there is normalized by resolving against the root, so maybe that should be done here and cache against the normalized url? There could still be an issue if you have a different resolution depending on the importer, but in that case, I think the urlToIdMap is going to end up with a wrong entry too because it isn't keying on importer.
There was a problem hiding this comment.
Need to add more comments, I confused myself with how much iteration this had
|
/ecosystem-ci run sveltekit |
|
📝 Ran ecosystem CI on
✅ |
There was a problem hiding this comment.
@sheremet-va let's merge this one so we can check other errors. I still think that different importers trying to import the same module could fail here though: #17957 (comment)
Description
Fix a use case when runner imports several different modules that rely on the same module. This is very race condition-reliant, so I wasn't able to make a test case for this, but it breaks SvelteKit, so at least we have the ecosystem test.
The error happens when the module is executed in one parallel call, but it's not resolved yet in another parallel call, so the second call invalidates the first call.