do not attach request decorations to the prototype#3795
do not attach request decorations to the prototype#3795cjihrig wants to merge 1 commit intohapijs:masterfrom cjihrig:3718
Conversation
This commit prevents request decorations from being attached to the Request prototype. This was leading to decorations being leaked between servers in the same process. Fixes: #3718
| // Give request access to all decorations without adding them to | ||
| // the Request prototype. | ||
| Object.setPrototypeOf(request, server._core._decorations.request); | ||
| Object.setPrototypeOf(server._core._decorations.request, proto); |
There was a problem hiding this comment.
Are there upsides to this versus the approach taken with requestApply?
There was a problem hiding this comment.
You mean looping over the decorations? There shouldn't be. Depending on the number of decorators, this might be faster since you're just adjusting some pointers vs. looping. I mostly went with this approach since it seemed more similar to the existing implementation (in case code was relying on decorations being on the prototype).
EDIT: After thinking about this more, I think I prefer the looping approach.
There was a problem hiding this comment.
Ah, that makes a lot of sense. I'm a little concerned by the performance warning MDN gives about setPrototypeOf() (which I can't personally confirm or deny for current-day V8). But also, setting the prototype of server._core._decorations.request to the same prototype on each request seems like it may be avoidable. Here's one idea for an alternative implementation that might have less question marks re: performance. master...devinivy:3718-alternative
There was a problem hiding this comment.
I like that approach better. I'm going to close this.
There was a problem hiding this comment.
I made a PR taking that approach in #3796—feel free to suggest improvements. I nabbed the test you had written here—if you'd like a proper "commit" credit for that, I'm happy to take the time.
|
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
2 participants
This commit prevents request decorations from being attached
to the
Requestprototype. This was leading to decorations beingleaked between servers in the same process.
Fixes: #3718
Note: I haven't benchmarked this change.