Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that fastify Request is encapsulated #2598

Closed
StarpTech opened this issue Oct 3, 2020 · 12 comments 路 Fixed by #5462
Closed

Ensure that fastify Request is encapsulated #2598

StarpTech opened this issue Oct 3, 2020 · 12 comments 路 Fixed by #5462
Labels
feature request New feature to be added v5.x Issue or pr related to Fastify v5
Milestone

Comments

@StarpTech
Copy link
Member

StarpTech commented Oct 3, 2020

馃殌 Feature Proposal

Fastify Request object is bound to the HTTP request lifecycle.

As a developer, I would expect that fastify creates a new Request object for every new request. While this is the case, fastify keeps object references across requests. This is due to the nature of prototype inheritance here.
This makes the code vulnerable to memory leaks: fastify/fastify-multipart#158 (comment)

We need to ensure that all properties are reset.

Motivation

As in other HTTP frameworks, the Request object is a new instance on every request. The developer shouldn't need to care about it. I propose to rethink that strategy because it's confusing and error-prone.

Example

fastify.decorateRequest('array', [])

fastify.post('/', async function (req, reply) {
    req.array.push(1)
    reply.code(200).send()
})

http.request(...) // array = [1]
http.request(...) // array = [1, 1]
.....

[ 1 ]
[ 1, 1 ]
[ 1, 1, 1 ]
[ 1, 1, 1, 1 ]
[ 1, 1, 1, 1, 1 ]
[ 1, 1, 1, 1, 1, 1 ]
@StarpTech StarpTech added the feature request New feature to be added label Oct 3, 2020
@StarpTech StarpTech changed the title Remove fastify request inheritance across requests Ensure that fastify Request is encapsulated Oct 3, 2020
@mcollina
Copy link
Member

mcollina commented Oct 3, 2020

It is not possible to achieve this reliably and in a performant way (I already stumbled on this a few times unfortunately). I think we should document this for v3 and maybe add a better API in v4.

@Eomm
Copy link
Member

Eomm commented Oct 5, 2020

We could support a factory option that will run for each request?

fastify.decorateRequest('array', function() { return [] }, { factory: true })

@mcollina
Copy link
Member

mcollina commented Oct 5, 2020

That'd slow things down considerably. Moreover, we have hooks for this kind of work and it's as is as adding an onRequest hook. Now, we could have the factory: true option to do all the boilerplate for us (using 'onRequest').

@StarpTech
Copy link
Member Author

StarpTech commented Nov 14, 2020

IMO this issue is really important. The circumstance that the instance is shared across requests is very confusing. This provides room for memory-leaks as shown above. I'd always prefer correctness over absolute speed. @mcollina do you mean that fastify.decorateRequest('array', []) register a shared onRequest hook to reset the data?

@zekth
Copy link
Member

zekth commented Nov 14, 2020

i'd assume that :

fastify.decorateRequest('array', function() { return [] }, { factory: true })

will result in subsequent

 const fn = function() { return [] };
  function onRequest (request, reply, done) {
    reply.array = fn()
    done()
  }

@mcollina
Copy link
Member

The problem is that we can't know how to creat a new instance identical to the value as can be of any custom type or class... and the only solution would be an unreliable (and slow) deep copy.

I think we should deprecate decorateRequest(prop, value) with any value that is not null or undefined. There is really no valid use case for that.

We could potentially add a new API that allows to set a factory for the instance value, but this is already covered by hooks.

@jsumners
Copy link
Member

jsumners commented Nov 14, 2020

I think we should deprecate decorateRequest(prop, value) with any value that is not null or undefined. There is really no valid use case for that.

Defaulting to an empty string is totally valid. Really, any primitive is valid. Why do you say it isn't?

@mcollina
Copy link
Member

Right! All value types are good, including functions. It's really objects, arrays, set and maps that are problematic.

@Eomm
Copy link
Member

Eomm commented Nov 14, 2020

It's really objects, arrays, set and maps that are problematic.

Agree on emitting a warning per these types

@kdrewes
Copy link

kdrewes commented Jul 28, 2023

Hello, I want to attempt to tackle this issue. Please let me know if this issue no longer needs assistance.

@mcollina mcollina added the v5.x Issue or pr related to Fastify v5 label Jul 29, 2023
@mcollina mcollina added this to the v5.0.0 milestone Jul 29, 2023
@mcollina
Copy link
Member

Sure!

Could you target the next branch?

How do you plan to solve this?

@gurgunday
Copy link
Member

gurgunday commented Apr 18, 2024

What's left here?

We now emit a warning if an object is decorated, and others have said complete encapsulation will hurt performance

Though I must admit I like the idea of knowing for sure that a completed request is gotten rid from memory, feels safer 馃槃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature to be added v5.x Issue or pr related to Fastify v5
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants