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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server hangs if a handler returns undefined #11

Closed
danielroe opened this issue Nov 22, 2020 · 6 comments
Closed

server hangs if a handler returns undefined #11

danielroe opened this issue Nov 22, 2020 · 6 comments

Comments

@danielroe
Copy link
Member

with automatic promisifying, if a handler returns undefined, the promise never resolves and the server hangs...

Is there a reason we don't immediately resolve(undefined) if the handler does?

danielroe added a commit that referenced this issue Nov 22, 2020
* includes commented-out test to match #11
@pi0
Copy link
Member

pi0 commented Nov 22, 2020

Nice catch, but something i'm afraid that, is we would continue to next middleware if handle uses an async (callback based) operation like this:

export default (req, res) {
  db.getUser((user) => {
     res.end(JSON.stringify(user))
  })
}

A legacy middleware like this would be broken.... I was supposing middleware (or better handle since has only two params) should always end request and so we listen to res.end. Do you have a case that it causes server to hang-up?

@danielroe
Copy link
Member Author

danielroe commented Nov 22, 2020

Very good point....! Will remove from #10

Yes, I like the simplicity of writing a handler like:

app.use('/api', (req) => {
  if (req.method !== 'GET') return

  return data
})

... but it's more important not to break promise-based handlers.

@pi0
Copy link
Member

pi0 commented Nov 22, 2020

I see... In that case we can introduce something like { legacy: true } and use modern syntax by default. Wdyt?

@danielroe
Copy link
Member Author

I would really like that, but do we lose the benefit of interop with express middleware where the user doesn't have control over each use() call? Could we create an entire App instance with { legacy: true } and have that behaviour apply to every handler...?

@danielroe danielroe reopened this Nov 22, 2020
@danielroe
Copy link
Member Author

danielroe commented Nov 22, 2020

For a repro of hang:

  1. Clone https://github.com/danielroe/jiti-h2-tests
  2. yarn && yarn dev
  3. curl http://localhost:3000/api/js/csr - (using h2) hangs
  4. curl http://localhost:3000/api/ts/csr - (using express) succeeds (e.g. falls back to Nuxt 404)

Was trying (and failed) to replicate issue with #13 - but oh well, at least they add a few more tests 🤷‍♂️

@pi0 pi0 closed this as completed in bb6cd4c Nov 23, 2020
@pi0 pi0 reopened this Nov 23, 2020
@pi0
Copy link
Member

pi0 commented Nov 23, 2020

Thanks for repro @danielroe! Actually it helped to find two issues:

  • When using h2 as a sub-app, we should use req.url not originalUrl to allow using relative paths (no /api/js in middleware itself)
  • Since connect detects app.handle, the hanging issue was happening so i made it hidden under app._handle (src)

@pi0 pi0 closed this as completed Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants