fix(platform-server): less aggressive ngServerMode cleanup #61106
Conversation
Other code may depend on `ngServerMode` and it might have been set globally / via a bundler. Forcing it to `undefined` in those situations can lead to hard debug issues where the only symptom is that "suddenly" browser-specific code paths run on the server and (obviously) break.
12bca02 to
4e69312
Compare
Other code may depend on `ngServerMode` and it might have been set globally / via a bundler. Forcing it to `undefined` in those situations can lead to hard debug issues where the only symptom is that "suddenly" browser-specific code paths run on the server and (obviously) break. PR Close #61106
|
This PR was merged into the repository by commit 06d6da3. The changes were merged into the following branches: main, 19.2.x, 20.0.x |
Other code may depend on `ngServerMode` and it might have been set globally / via a bundler. Forcing it to `undefined` in those situations can lead to hard debug issues where the only symptom is that "suddenly" browser-specific code paths run on the server and (obviously) break. PR Close #61106
| platform.onDestroy(() => { | ||
| globalThis['ngServerMode'] = undefined; | ||
| }); |
There was a problem hiding this comment.
Hi @alan-agius4
Reading and overwriting a global shared state sounds like being prone to race conditions, when it happens for various independent incoming requests. Am I missing something?
I'm worried by seeing that a global shared state being accessed and overwritten by specific a instances of Angular app (on its creation and on its destroy), because there may exist many parallel requests coming to the server, meaning many parallel renderings, meaning many instances of Angular app, meaning many calls of platformServer() (please correct me if my reasoning is wrong...). I guess it may lead to race conditions on reading/writing this global shared state globalThis['ngServerMode'].
Let's imagine the following example:
- a first request comes for the URL
Aand aPlatformRefis created for this request with callingplatformServer(). It's a first request, so the global shared statengServerModeisundefined. In other words,const noServerModeSet = typeof ngServerMode === 'undefined';is evaluated totrue. Now a global state is overwritten:globalThis['ngServerMode']is set totrue. - While the rendering for the request
Ais still pending, in the meantime comes a new requestB, and a newPlatformRefis created for the rendering of the requestB, with callingplatformServer()to create a new instance of Angular appB. At this momentngServerModeis true (becasue it was set by the creation ofPlatformRefA). In other wordsconst noServerModeSet = typeof ngServerMode === 'undefined';is evaluated tofalse, so no side effects will happen on creation nor destroying thisPlatformRefB. - Some time passes and the server-side rendering for request
Afinishes, so itsPlatformRefis destroyed and the global shared state is overwritten to:globalThis['ngServerMode'] = undefined; - But(!) the rendering of the request B is still pending, and if any logic in that rendering attempts to read the global shared state
globalThis['ngServerMode']now, then it will get a valueundefinedinstead oftrue.
As far as I understand from the simple Angular codebase search, there are plenty of places in the Angular logic that attempt to read the ngServerMode global shared state. So I understand that the remaining rendering for the request B potentnially can have bugs.
I guess perhaps I'm missing something obvious and important in my reasoning. Then please let me know, I'm happy to learn.
There was a problem hiding this comment.
The ngServerMode is replaced with a constant at build time by the bundler.
There was a problem hiding this comment.
Oh, I wasn't aware of it. Thank you for letting me know! 👍
If not a problem, let me ask to understand even better:
If ngServerMode is set at build time, then why/when it makes sense to set this value at runtime inside the logic of the function platformServer()? (What is the usecase)
|
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. |
Other code may depend on
ngServerModeand it might have been set globally / via a bundler. Forcing it toundefinedin those situations can lead to hard debug issues where the only symptom is that "suddenly" browser-specific code paths run on the server and (obviously) break.Note: This is the less aggressive version that still forces
ngServerModeto undefined if that's how we found it. I think it would also be reasonable to remove this cleanup all together but I tried to err on the side of caution.