Complete handler chain if res is sent #166

jakeorr · 2021-12-10T21:47:33Z

This change allows handler(res, req) to resolve for middlewares that don't call next, but end the response.

Example:

// index.page.js
const handler = nc()
  .get(async (req, res) => {
    res.status(200).send('hello!')
  });
export default handler;

import { createMocks } from 'node-mocks-http';
import handler from './index.page';

const { req, res } = createMocks({ method: 'GET', url: '/' });
await handler(req, res);
// With this change we can now get here.

This change allows `handler(res, req)` to resolve for middlewares that don't call next, but end the response.

changeset-bot · 2021-12-10T21:47:35Z

⚠️ No Changeset found

Latest commit: 0358b74

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codecov · 2021-12-10T21:47:55Z

Codecov Report

Merging #166 (0358b74) into master (14aa547) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #166   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           81        88    +7     
=========================================
+ Hits            81        88    +7

Impacted Files	Coverage Δ
src/index.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14aa547...0358b74. Read the comment docs.

jakeorr · 2021-12-10T21:50:44Z

Thanks for the handy library! Let me know if you would like anything further on the PR. 😄

hoangvvo · 2021-12-12T04:13:26Z

src/index.js

@@ -67,7 +67,9 @@ export default function factory({
    if (attachParams) req.params = params;
    let i = 0;
    const len = handlers.length;
-    const loop = async (next) => handlers[i++](req, res, next);
+    const loop = async (next) => Promise.resolve(handlers[i++](req, res, next))


In this case, if the handler is synchronous (not async) and throws, it will not be able to process the proceeding .then / .catch

Hi, thanks for taking a look and good catch on the error handling discrepancy. I just pushed a change so that errors thrown by sync middleware and async middleware rejecting will both be handled in one spot.

This change causes errors for handlers (either synchronous or asynchronous) to be handled in the same way.

jakeorr · 2022-01-03T18:31:53Z

Hi @hoangvvo. Happy New Year! Just wanted to check if you needed anything else on this change or had any further thoughts on it. Thanks!

hoangvvo · 2022-01-04T07:23:16Z

src/index.js

-    const loop = async (next) => handlers[i++](req, res, next);
+    const loop = async (next) => {
+      try {
+        await Promise.resolve(handlers[i++](req, res, next)); 


Promise.resolve() is not necessary here.

I added the Promise.resolve() for the case of handlers that do not return a promise.

It is still not needed, if handler() does not return a Promise, await ... will resolve immediately. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await

If the value of the expression following the await operator is not a Promise, it's converted to a resolved Promise.

Ah, you are so right! Thanks for the correction.

hoangvvo · 2022-01-04T07:24:08Z

Happy new year! Thanks for the PR and sorry for the late review. However, I don't think this breaking change is possible at this point. Since checking if response is sent is not really reliable (for cases like redirection, multiple res sent, multipart response, etc. and edge cases like race-condition etc.)

jakeorr · 2022-01-04T17:22:53Z

Happy new year! Thanks for the PR and sorry for the late review. However, I don't think this breaking change is possible at this point. Since checking if response is sent is not really reliable (for cases like redirection, multiple res sent, multipart response, etc. and edge cases like race-condition etc.)

No problem, I've been working off a fork, so it's not a big deal if this doesn't get merged.

To your point about checking if the response is sent, I hadn't thought about those details you mentioned. Is there a concern that isResSent will return true when the response really hasn't been sent? If you're only worried about the other way around, where this change wouldn't detect the response as sent, then it's no different than it was previously. It wouldn't call done in the new spot and the existing code would continue working as it has been.

hoangvvo · 2022-02-03T12:22:05Z

Sorry about the wait, I just have some time to revisit this PR today. I think I finally agree that this issue should be resolved. However, one concern is that sometimes I want to do some post processing after the response is sent. In that case, it would not really make sense to end the promise there. What do you think about that case. Example:

const handler = nc()
  .get(async (req, res, next) => {
    res.status(200).send('hello!');
    next();
  })
  .all(async (req, res) => {
    // it would have been resolved before this step
    await logPostRequestToDatabase(req);
  };
export default handler;

jakeorr · 2022-02-03T20:09:36Z

Sorry about the wait, I just have some time to revisit this PR today. I think I finally agree that this issue should be resolved. However, one concern is that sometimes I want to do some post processing after the response is sent. In that case, it would not really make sense to end the promise there. What do you think about that case. Example:
const handler = nc()
  .get(async (req, res, next) => {
    res.status(200).send('hello!');
    next();
  })
  .all(async (req, res) => {
    // it would have been resolved before this step
    await logPostRequestToDatabase(req);
  };
export default handler;

No worries! Thanks for spending some time on it. Good point about running functions after the response is sent. My initial thought it to call next instead of done if the response is sent, but testing that, I'm seeing next called extra times which causes done to be called multiple times. So, do we do something like this?

isResSent(res) && !next.wasCalled && next();

I'll need to spend some time setting up something to detect if next was called. Any initial thoughts on that idea?

hoangvvo · 2022-02-04T03:59:07Z

I would say that the current design choice for this library is not ideal. I don't really find a way to really work this out reliably.

Also consider this case below:

const handler = nc()
  .get(async (req, res, next) => {
    somePromise().then(() => res.end()); // <- notice this is not returned and not await for
    // or setTimeout(() => res.end(), 0);
  })

After the handler is resolved. isResSent() is obviously false because res.end() has not been called (it calls in the next tick, or after the promise is resolved, which the function does not wait for). With this the check of isResSent() then resolve promise will never be triggered.

hoangvvo · 2022-02-04T04:11:24Z

At this point, it just makes the most sense to not returning a promise in a handler, since we never know when that promise is supposed to be resolved.

hoangvvo · 2022-02-04T04:13:24Z

The other way to solve this is to follow this pattern #148 (similar to Koa or Go-Gin). With this we can for sure know the promise will be resolve (since next() is the call to next function, keep awaiting them and when it resolves, it is the time to do so in the handler). However, this breaks current support with express middleware.

hoangvvo · 2022-02-04T04:44:40Z

Sorry about the wait, I just have some time to revisit this PR today. I think I finally agree that this issue should be resolved. However, one concern is that sometimes I want to do some post processing after the response is sent. In that case, it would not really make sense to end the promise there. What do you think about that case. Example:
const handler = nc()
  .get(async (req, res, next) => {
    res.status(200).send('hello!');
    next();
  })
  .all(async (req, res) => {
    // it would have been resolved before this step
    await logPostRequestToDatabase(req);
  };
export default handler;
No worries! Thanks for spending some time on it. Good point about running functions after the response is sent. My initial thought it to call next instead of done if the response is sent, but testing that, I'm seeing next called extra times which causes done to be called multiple times. So, do we do something like this?

isResSent(res) && !next.wasCalled && next();

I'll need to spend some time setting up something to detect if next was called. Any initial thoughts on that idea?

Funny enough this is not the first time the issue brought up. senchalabs/connect#1042 (so the same case in Express.js) was here long before and the fact that it is not yet solved indicating this is a problematic issue. It is probably due to the bad design choice of Express.js (therefore, of this library too) that makes this nearly impossible to solve.

hoangvvo · 2022-02-04T08:24:57Z

I gave an attempt to resolve this in #178. Could you check it out whenever you have the time?

My solution is to listen to the event res.on("close") https://nodejs.org/api/http.html#event-close_1

jakeorr · 2022-02-05T01:06:01Z

Thanks for your thoughts on the matter and good points about the design limitations. I had a brief look at your PR, and I'll have time to take a closer look at it tomorrow.

hoangvvo · 2022-02-09T16:09:48Z

Closing in favor of #178. Thanks for the PR.

Complete handler chain if res is sent

2dc97b6

This change allows `handler(res, req)` to resolve for middlewares that don't call next, but end the response.

hoangvvo reviewed Dec 12, 2021

View reviewed changes

use try/catch + await to generalize error handling

c837eee

This change causes errors for handlers (either synchronous or asynchronous) to be handled in the same way.

hoangvvo reviewed Jan 4, 2022

View reviewed changes

mactkg mentioned this pull request Feb 2, 2022

How to test NextJS endpoints #84

Open

remove unecessary Promise.resolve()

0358b74

hoangvvo closed this Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete handler chain if res is sent #166

Complete handler chain if res is sent #166

jakeorr commented Dec 10, 2021

changeset-bot bot commented Dec 10, 2021 •

edited

Loading

codecov bot commented Dec 10, 2021 •

edited

Loading

jakeorr commented Dec 10, 2021

hoangvvo Dec 12, 2021

jakeorr Dec 13, 2021

jakeorr commented Jan 3, 2022

hoangvvo Jan 4, 2022

jakeorr Jan 4, 2022

hoangvvo Feb 3, 2022

jakeorr Feb 3, 2022

hoangvvo commented Jan 4, 2022

jakeorr commented Jan 4, 2022

hoangvvo commented Feb 3, 2022

jakeorr commented Feb 3, 2022 •

edited

Loading

hoangvvo commented Feb 4, 2022 •

edited

Loading

hoangvvo commented Feb 4, 2022

hoangvvo commented Feb 4, 2022 •

edited

Loading

hoangvvo commented Feb 4, 2022

hoangvvo commented Feb 4, 2022 •

edited

Loading

jakeorr commented Feb 5, 2022

hoangvvo commented Feb 9, 2022

Complete handler chain if res is sent #166

Complete handler chain if res is sent #166

Conversation

jakeorr commented Dec 10, 2021

changeset-bot bot commented Dec 10, 2021 • edited Loading

⚠️ No Changeset found

codecov bot commented Dec 10, 2021 • edited Loading

Codecov Report

jakeorr commented Dec 10, 2021

hoangvvo Dec 12, 2021

Choose a reason for hiding this comment

jakeorr Dec 13, 2021

Choose a reason for hiding this comment

jakeorr commented Jan 3, 2022

hoangvvo Jan 4, 2022

Choose a reason for hiding this comment

jakeorr Jan 4, 2022

Choose a reason for hiding this comment

hoangvvo Feb 3, 2022

Choose a reason for hiding this comment

jakeorr Feb 3, 2022

Choose a reason for hiding this comment

hoangvvo commented Jan 4, 2022

jakeorr commented Jan 4, 2022

hoangvvo commented Feb 3, 2022

jakeorr commented Feb 3, 2022 • edited Loading

hoangvvo commented Feb 4, 2022 • edited Loading

hoangvvo commented Feb 4, 2022

hoangvvo commented Feb 4, 2022 • edited Loading

hoangvvo commented Feb 4, 2022

hoangvvo commented Feb 4, 2022 • edited Loading

jakeorr commented Feb 5, 2022

hoangvvo commented Feb 9, 2022

changeset-bot bot commented Dec 10, 2021 •

edited

Loading

codecov bot commented Dec 10, 2021 •

edited

Loading

jakeorr commented Feb 3, 2022 •

edited

Loading

hoangvvo commented Feb 4, 2022 •

edited

Loading

hoangvvo commented Feb 4, 2022 •

edited

Loading

hoangvvo commented Feb 4, 2022 •

edited

Loading