Fix 677 #692

delvedor · 2018-01-20T16:19:15Z

This pr fixes #677.
In addition will release the reusify holder which is running the hooks, in this way the memory usage will not increase during time.

Related: delvedor/fast-iterator#8 and fastify/middie#17.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

mcollina

LGTM

delvedor · 2018-01-20T16:47:28Z

I'll wait for fastify/middie#17 before merge this.

nwoltman · 2018-01-20T16:49:36Z

fastify.js


-  function hookIterator (fn, state, next) {
+  function hookIterator (fn, state, next, release) {
+    if (state.res.finished === true) return release()


If the hook sends a stream, will res.finished be true here? You may need to use res.headersSent instead.

I'll test this tomorrow :)

nwoltman · 2018-01-20T17:14:31Z

There should probably be some documentation to tell people using the callback version of hooks/middleware that if they send a response early, they must still call next() or else there will be a memory leak.

jsumners · 2018-01-20T18:36:19Z

test/hooks-async.js

    })
  })
+
+  test('onRequest hooks should be able to block a request', t => {


Maybe these cases should be worded: "should be able to terminate a request"? The word "block" seems to imply something more sinister to me.

jsumners

The associated issue speaks specifically about using async handlers. Can we get a test that shows how those should work?

delvedor · 2018-01-20T18:55:58Z

@jsumners I've already written them, check again :P

@nwoltman 👍

jsumners · 2018-01-20T19:17:10Z

@delvedor oops, I see.

jsumners

LGTM

delvedor · 2018-01-21T21:25:21Z

I've added a test case for streams. The issue is that res.finished or res.headersSent or res.connection._writableState.writing are always false, even if we call next inside a setImmediate or inside a nextTick.
At the moment the solution is to use res.once('finish', next), in this way you will not encounter errors. Everything has been documented.

nwoltman · 2018-01-21T22:03:58Z

docs/Hooks.md

+
+// async api
+fastify.addHook('preHandler', async (request, reply) => {
+  reply.code(401).send(new Error('Unauthorized'))


A better example would be to send something that isn't an Error because you can already do this with next(new Error('Unauthorized') for callbacks and throw new Error('Unauthorized') for async.

nwoltman · 2018-01-21T22:07:42Z

@delvedor Thanks for figuring out how to make this work with streams :)

There's 1 case missing though. If the very last hook/middleware sends a response, then you also need to check res.finished in the callback for the hooks (because hookIterator isn't called after the last hook).

delvedor · 2018-01-21T22:16:03Z

@nwoltman nice catch! :)

delvedor · 2018-01-21T22:19:59Z

LOL, 42 additions of docs, 12 of core library and 555 of tests :D

nwoltman

Almost there! Just one last bit for the middlewares.

nwoltman · 2018-01-21T22:44:47Z

fastify.js

  }

  function middlewareCallback (err, state) {
+    if (state.res.finished === true) return


Same thing in onRunMiddlewares

There should be no need for that test. The middlewares are handled by middie, and the res.finished check is performed before to call the iterator or complete. :)
https://github.com/fastify/middie/blob/master/middie.js#L70-L77

Oh that's included in middie. Cool!

nwoltman

LGTM 👍

mcollina

The code is ok. I would not document the stream parts of preHandler as it should not be used that way, especially with async/await.

mcollina · 2018-01-22T08:12:22Z

docs/Hooks.md

 Note: If you change the payload, you may only change it to a `string`, a `Buffer`, a `stream`, or `null`.

+### Respond to a request from an hook
+If need you can respond to a request before you reach the route handler, an example could be an authentication hook. If you are using `onRequest` or a middleware just use `res.end`, if you are using the `preHandler` hook use `reply.send`. Remember to always call `next` if you are using the standard hook api, if you are working with *async* hooks it will be done automatically by Fastify.


This is not true. If you 'forget' to call next, the only harm you will cause is to potentially increase the memory consumption, and you will prevent one of our optimizations to kick in.

@delvedor @mcollina Sorry for suggesting this. I misunderstood how reusify works. I now see that if you don't call holder.release(obj) the object just won't be reused and it will be garbage-collected (so it doesn't cause a memory leak).

mcollina · 2018-01-22T08:13:31Z

docs/Hooks.md

+  const stream = fs.createReadStream('some-file', 'utf8')
+  stream.pipe(res)
+  res.once('finish', next)
+})


Be careful that this code is substantially wrong because it will leak a file descriptor if stream errors. How about using pump?

mcollina · 2018-01-22T08:16:46Z

docs/Hooks.md

+fastify.addHook('preHandler', (request, reply, next) => {
+  const stream = fs.createReadStream('some-file', 'utf8')
+  reply.send(stream)
+  reply.res.once('finish', next)


We should really not be needing this.
We should not really be encouraging anyone to do this, so it's better if we do not document this behavior.
Doing this in their code is substantially wrong.

So your suggestion is to remove the documentation about respond with streams inside the hooks?

I would say so. We definitely need a better story for this hook, and there is solid work already done here which I do not want to block. Can you open a separate issue about replying with streams in a 'preHandler'?

Ok, I've updated the docs.

mcollina · 2018-01-22T08:16:54Z

docs/Hooks.md

+    const stream = fs.createReadStream('some-file', 'utf8')
+    stream.pipe(res)
+    res.once('finish', resolve)
+  })


this is even worse.

github-actions · 2022-02-20T00:20:09Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

delvedor added 3 commits January 20, 2018 17:14

Fixes #677

869cf0e

Bumped dependencies

785dcf0

Updated test

696f860

delvedor added enhancement bugfix Issue or PR that should land as semver patch labels Jan 20, 2018

delvedor requested review from jsumners, mcollina and nwoltman January 20, 2018 16:19

mcollina approved these changes Jan 20, 2018

View reviewed changes

delvedor mentioned this pull request Jan 20, 2018

Update fast-iterator to the latest version 🚀 #691

Closed

delvedor added this to the v1.0.0 milestone Jan 20, 2018

nwoltman reviewed Jan 20, 2018

View reviewed changes

Updated middie

a8e69dc

jsumners reviewed Jan 20, 2018

View reviewed changes

jsumners approved these changes Jan 20, 2018

View reviewed changes

delvedor added 2 commits January 21, 2018 22:22

Support end of stream also in preHandler

fe9931c

Updated test

20ba13a

nwoltman reviewed Jan 21, 2018

View reviewed changes

Updated docs

edd1a19

delvedor force-pushed the fix-677 branch from eb8afc2 to edd1a19 Compare January 21, 2018 22:08

delvedor added 2 commits January 21, 2018 23:15

Check if the response has finished after the hooks execution

caa3a72

Updated test

62396ae

Merge branch 'master' into fix-677

2dde951

nwoltman reviewed Jan 21, 2018

View reviewed changes

nwoltman approved these changes Jan 21, 2018

View reviewed changes

mcollina approved these changes Jan 22, 2018

View reviewed changes

Updated docs

d889a40

delvedor merged commit d89079c into master Jan 22, 2018

delvedor deleted the fix-677 branch January 22, 2018 20:44

github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2022

Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023

Uh oh!

Fix 677 #692

Fix 677 #692

Uh oh!

Conversation

delvedor commented Jan 20, 2018

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

delvedor commented Jan 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwoltman commented Jan 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

delvedor commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsumners commented Jan 20, 2018

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

delvedor commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwoltman commented Jan 21, 2018

Uh oh!

delvedor commented Jan 21, 2018

Uh oh!

delvedor commented Jan 21, 2018

Uh oh!

nwoltman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delvedor Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwoltman left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 20, 2022

Uh oh!

Reviewers

Assignees

Labels

delvedor commented Jan 20, 2018 •

edited

Loading

delvedor Jan 21, 2018 •

edited

Loading