Any reason for awaiting complete requests before sending responses?

Hi, I have a question about this part of the code. Is there any (specific) reason for awaiting a complete request before sending a response?

finalhandler/index.js

Lines 300 to 310 in 15e78ca

    
           if (isFinished(req)) { 
        
             write() 
        
             return 
        
           } 
        
           // unpipe everything from the request 
        
           unpipe(req) 
        
           // flush the request 
        
           onFinished(req, write) 
        
           req.resume()

I think this would be a potential risk in some edge case, for example from the README:

var finalhandler = require('finalhandler')
var http = require('http')

var server = http.createServer(function (req, res) {
  var done = finalhandler(req, res)
  done()
})

server.listen(3000)

This should always return 404 immediately for any requests, but the following request can cause the server to be hung up:

curl -X$ANY_METHOD http://localhost:3000 -H content-length:1

I know this hardly happens and is the clients' fault unless an intentional attack; plus, we can set a timeout for general requests.

However, I think there is no need to await the body or a complete request for the requests that we don't expect (such as 404) in order to return error responses immediately without being hung up by omitting to do that.

@shqld

Hi @shqld the reason is that in Node.js, if there is a large incoming body request that has not been fully read off, Node.,js will just end up destroying the connection and the client will never get the reply. This wait ensures that the client will indeed recieve the response. You can find tests in the test suite around this behavior and you will find that removing this will cause them to fail.

As far as your scenario, that would happen to any program that attempts to read the body in Node.js; the Node.js web server allows you to configure how long you are willing to wait for data from the client when the client indicates there is data to read: https://nodejs.org/dist/latest-v14.x/docs/api/http.html#http_server_settimeout_msecs_callback

It is also important to keep in mind that HTTP requests are not as simple as one TCP connection = one HTTP request; for example HTTP/1.1 is keep-alive by default. Without reading the body of request 1 off the buffer and simply just replying, Node.js may never actually process request 2 (depending on the size of request 1 due to various internal Node.js buffer sizes) and then request 2 will end up hanging.

Understood completely. Thank you so much for your quick and so lucid reply!

@dougwilson

@dougwilson By the way, what if req.destroy() right after write()? (I'll check by myself though)

It is also important to keep in mind that HTTP requests are not as simple as one TCP connection = one HTTP request; for example HTTP/1.1 is keep-alive by default. Without reading the body of request 1 off the buffer and simply just replying, Node.js may never actually process request 2 (depending on the size of request 1 due to various internal Node.js buffer sizes) and then request 2 will end up hanging.

The issue is that req.destroy() will also destroy the socket. Even if you are replying with a 404 you still want to ideally process the next request. It's also a race condition as Node.js will try and cork writes together, so destroying the socket right away before the async operation may not always even get the response sent out.

I see... thanks a lot [at]dougwilson.

Navigation Menu

Repository navigation

Any reason for awaiting complete requests before sending responses? #36

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (isFinished(req)) {
	write()
	return
	}

	// unpipe everything from the request
	unpipe(req)

	// flush the request
	onFinished(req, write)
	req.resume()

Any reason for awaiting complete requests before sending responses? #36

Description

Activity

dougwilson commented on Mar 4, 2021

shqld commented on Mar 4, 2021

shqld commented on Mar 5, 2021

dougwilson commented on Mar 5, 2021

shqld commented on Mar 5, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions