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

fix: properly handle prematurely closed requests #4772

Merged

Conversation

zeeshanakram3
Copy link
Contributor

addresses #4760

Problem

Sometimes a request can be prematurely closed by client or due to bad network conditions. Whenever that happens, an event handler on response object (res), manually closes the response by calling res.end(). However, the problem is that even if the response closes the the control flow is passed the request handlers which then tries to set headers on a closed response, and hence the error.

How to reproduce

In routeWrapper function, manually close the response by calling res.socket?.destory(),

  protected routeWrapper(handler: express.RequestHandler) {
    return async (req: express.Request, res: express.Response, next: express.NextFunction): Promise<void> => {
      // Fix for express-winston in order to also log prematurely closed requests
      
+     res.socket.?destroy()
      res.on('close', () => {
        res.locals.prematurelyClosed = !res.writableFinished
        res.end()
      })
      
+     await sleep(1000) // sleep for 1 second to make sure that request does not finish before socket destroy event is emitted
      try {
        await handler(req, res, next)
      } catch (err) {
        next(err)
      }
    }
  }

@vercel
Copy link

vercel bot commented May 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
pioneer-testnet ⬜️ Ignored (Inspect) Jul 12, 2023 4:30am

@zeeshanakram3 zeeshanakram3 added argus Argus distributor node origo Origo Release labels May 24, 2023
* closed prematurely). Otherwise, the request handler would try to set header or send the response
* again, which would result in an error e.g., "Cannot set headers after they are sent to the client"
* */
if (!res.headersSent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary. My hunch is that the root cause of this behavior is the invocation of res.end() in the close event handler on line 26. It just looks wrong. I'll test and confirm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirmed that by removing the res.end() and the newly added conditional on res.headerSent, invoking socket.destroy(), execution does not reach invokation of await handler(req, res, next).
Also confirmed the log correctly includes the meta information for prematurelyClosed:

2023-06-12 18:57:17 2023-06-12 14:57:17:5717 PublicApi http: HTTP GET /api/v1/status
2023-06-12 18:57:17 {
2023-06-12 18:57:17     "meta": {
2023-06-12 18:57:17         "req": {
2023-06-12 18:57:17             "url": "/api/v1/status",
2023-06-12 18:57:17             "headers": {
2023-06-12 18:57:17                 "host": "localhost:3334",
2023-06-12 18:57:17                 "user-agent": "curl/8.1.2",
2023-06-12 18:57:17                 "accept": "*/*"
2023-06-12 18:57:17             },
2023-06-12 18:57:17             "method": "GET",
2023-06-12 18:57:17             "httpVersion": "1.1",
2023-06-12 18:57:17             "originalUrl": "/api/v1/status",
2023-06-12 18:57:17             "query": {}
2023-06-12 18:57:17         },
2023-06-12 18:57:17         "res": {
2023-06-12 18:57:17             "statusCode": 200
2023-06-12 18:57:17         },
2023-06-12 18:57:17         "responseTime": 60,
2023-06-12 18:57:17         "prematurelyClosed": true
2023-06-12 18:57:17     }
2023-06-12 18:57:17 }

Copy link
Contributor

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simpler solution is to just not call res.end() in the closed event handler.

@mnaamani mnaamani self-requested a review July 12, 2023 06:31
@mnaamani mnaamani merged commit 2ea8735 into Joystream:master Jul 12, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
argus Argus distributor node origo Origo Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants