Multiple Middleware on Route Hangs about hyper-express HOT 35 CLOSED

I'm working on a mac at the moment which is also unix. I will try to fix it, in the mean time can you run that script locally and hit it with postman?

from hyper-express.

We should definitely do that as well. Im sure @alexhultman would appreciate it.

from hyper-express.

Here is the example in JS for ease of copy/paste:

import {  Server } from 'hyper-express'

const app = new Server()

app.post(
  '/echo',
  async (req, res, next) => {
    req.body = await req.json()
    next()
  },
 (req, res, next) => {
    res.locals.data = req.body
    next()
  },
 (req, res) => {
    res.status(200).json(res.locals.data)
  }
)

app.listen(3000)

from hyper-express.

@SupremeTechnopriest I can add this to tests but one thing I noticed with your code is that in the first middleware. You have an async middleware but you also call "next()". For async middlewares, you do not have to call next() as the Promise returned by the async function is awaited by HyperExpress.

Can you try removing the next() from the async function to see if it is resolved.

from hyper-express.

Sure let me give that a go.

from hyper-express.

So when I remove the next() call I get the following error:

HyperExpress: Double middleware execution detected! You have a bug where one of your middlewares is calling both the next() callback and also resolving from a Promise/async callback. You must only use one of these not both.

app.post(
    '/echo',
    async (req: Request, res: Response, next: MiddlewareNext) => {
      req.body = await req.json()
    },
    (req: Request, res: Response, next: MiddlewareNext) => {
      res.locals.data = req.body
      next()
    },
    (req: Request, res: Response) => {
      res.status(200).json(res.locals.data)
    }
  )

With the next() call in the async, it returns the response once and then throws an ECONNRESET. Maybe some logic is inverted?

from hyper-express.

It's worth noting that even with the error throwing (next removed) the second request still hangs.

from hyper-express.

Were you able to recreate it in the tests?

from hyper-express.

@SupremeTechnopriest I just added a Test for this and pushed it. Can you clone/pull the latest commit and run the tests on your machine? It ran fine on my side and the Double middleware execution only occurs If you mix up async/sync middlewares and attempt to iterate forwards twice from a single middleware.

If you wish to debug further, try console logging the Server.middlewares property and Server.routes (Iterate through all of the route instances and you can log Route.middlewares property) and try and see If there is any middleware you don't recognize.

Also, try logging the Error object directly from the global error handler to see If you can see a trace/line number which points to your middleware that causes the violation.

from hyper-express.

let me give it a go.

from hyper-express.

So I had another async middleware that was calling next. Pulled that out and I stopped getting the error, but the main issue is still persisting (the hanging on the second call). I create a very minimal test file to make sure its not related to my code:

const { Server } = require('hyper-express')

const app = new Server()

app.get('/', (_, res) => {
  res.status(200).send('Hello World')
})

app.post(
  '/echo',
  async (req) => {
    req.body = await req.json()
    return
  },
  (req, res, next) => {
    res.locals.data = req.body
    next()
  },
  (_, res) => {
    res.status(200).json(res.locals.data)
  }
)

app.listen(3000).then(() => {
  console.log('Listening')
})

The get method is throwing this error and crashing the server:

/node_modules/hyper-express/src/components/http/Request.js:93
        this.#raw_response.pause();
                           ^

Error: Invalid access of discarded (invalid, deleted) uWS.HttpResponse/SSLHttpResponse.
    at Request.pause (/Users/rl/Sites/projects/edgemesh/v4/core-api/node_modules/hyper-express/src/components/http/Request.js:93:28)
    at /Users/rl/Sites/projects/edgemesh/v4/core-api/node_modules/hyper-express/src/components/http/Request.js:160:46

The post method with the request body { "foo": "bar" } echos back the body on the first call and then hangs on the second. If I cancel the second call and submit a third, it returns. Then it hangs... and so on.

Could you run this file locally and test the routes with postman and confirm you get the same results?

from hyper-express.

So just to confirm you are testing these routes locally from Postman to trigger this error consistently on every other request?

from hyper-express.

Hey, was able to figure out the bug. It ended up being a weird behavior with Readable streams that I looked over when implementing them which was leading to some requests hanging due to the stream being in a paused state by default instead of flowing. This bug has been fixed in 6.0.3, so try upgrading to 6.0.3 to see If your bug still persists.

from hyper-express.

Hey @kartikk221!

That definitely fixed the middleware issue, but the route without middleware is still throwing that Invalid Access error. Yes I was testing with postman.

const { Server } = require('hyper-express')

const app = new Server()

app.get('/', (_, res) => {
  res.status(200).send('Hello World')
})

app.listen(3000).then(() => {
  console.log('Listening')
})

GET /

/node_modules/hyper-express/src/components/http/Request.js:95
            this.#raw_response.pause();
                               ^

Error: Invalid access of discarded (invalid, deleted) uWS.HttpResponse/SSLHttpResponse.
    at Request.pause (/Users/rl/Sites/projects/edgemesh/v4/core-api/node_modules/hyper-express/src/components/http/Request.js:95:32)
    at /Users/rl/Sites/projects/edgemesh/v4/core-api/node_modules/hyper-express/src/components/http/Request.js:183:46

from hyper-express.

That definitely fixed the middleware issue, but the route without middleware is still throwing that Invalid Access error. Yes I was testing with postman.

I added tests for the code you provided in the above scenarios to the repository. Are you able to run all of the tests for HyperExpress normally in your environment?

from hyper-express.

Im still having issues with that submodule. Let me see if I can resolve it.

from hyper-express.

git submodule update

Please make sure you have the correct access rights
and the repository exists.
fatal: clone of '[email protected]:kartikk221/hyper-express-session.git' into submodule path '/Users/rl/Sites/forks/hyper-express/middlewares/hyper-express-session' failed
Failed to clone 'middlewares/hyper-express-session' a second time, aborting

Maybe I should just clone it in?

from hyper-express.

I also receive this error for some reason. For me on Windows, this is usually resolved by using Git Bash program.

from hyper-express.

Also, from the error trace you provided above, It seems like the error occurs here Request.js which essentially means that there is still Request body data being received but the response has already been sent. This makes no sense to me because uWebsockets.js will stop downloading any data If the response has been sent.

from hyper-express.

Yeah this is definitely weird, because it wasn't happening before and just randomly started happening yesterday.

from hyper-express.

I'm working on a mac at the moment which is also unix. I will try to fix it, in the mean time can you run that script locally and hit it with postman?

Runs fine on my end GIF but there is an additional boolean check I can add in the body streamer to potentially prevent your problem based on the error trace.

from hyper-express.

So weird.... I wonder why we are getting different results.

from hyper-express.

So weird.... I wonder why we are getting different results.

I just pushed a commit with stricter checks on the Request's body stream. Try pulling the new commit and testing to see If those checks fix your situation.

Also, are you making the Postman request to HyperExpress serving requests from a remote machine or from your mac itself?

from hyper-express.

Did you publish it to npm or should pull upstream and link? Also I manually cloned the submodule and tests all pass.

from hyper-express.

Did you publish it to npm or should pull upstream and link? Also I manually cloned the submodule and tests all pass.

Just pushed it to this respository not NPM, so you should be able to pull upstream and run the tests again along with your code.

from hyper-express.

Still the same error.

from hyper-express.

Still the same error.

So all of the tests pass but your GET endpoint that serves "Hello World" throws the discarded access error? Did anything about the error change in terms of the trace? Is it the exact same line that calls the Request.pause() method leading to the error?

from hyper-express.

Okay I figured it out. It happens when you send a body on a GET method. I accidentally had the body from an old request in postman. Maybe we should protect against this?

from hyper-express.

You can recreate it with this curl:

curl --location --request GET 'localhost:3000/' \
--header 'Content-Type: application/json' \
--data-raw '{
    "t": 1234,
    "foo": "bar"
}'

from hyper-express.

Only POST, PUT and PATCH methods should attempt to get the body buffer. As it stands someone could easily DOS attack the server by sending GET methods with a request body.

from hyper-express.

Just pushed a commit which adds a hard boolean check to stop reading any body chunks as soon as response has ended.
Good catch on the error. Here's the reasons for how this bug even got created:

• uWebsockets.js for some reason continues to serve body array buffers to the onData() handler even after the response has been discarded.
• The stream flow checks in the onData() handler which are supposed to halt any chunks from being processed depend on the fact that the body stream was actually being consumed at some point of the life cycle which doesn't occur because GET requests are not supposed to have body data anyways.

I've implemented an additional hard boolean check to the onData() handler which halts processing of any incoming body chunks once the Response component sends out the response. This should also be a more future proof solution instead of hard-coding the exact HTTP methods and If for whatever reason you wanted to consume body data from GET requests, you could with HyperExpress.

from hyper-express.

Cool! Let me try it out.

from hyper-express.

Looks good to me! Ship it!

from hyper-express.

uWebsockets.js for some reason continues to serve body array buffers to the onData() handler even after the response has been discarded.

This should also be a more future proof solution

@kartikk221 wouldn't the future-proof solution be to open a bug report upstream with uWebsockets.js 🤔 🤣

from hyper-express.

Absolutely, this is something that I thought was already handled at the low-level with uWebsockets.js but I guess not and hence this was even a problem in the first place.

from hyper-express.