nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
105.64k stars 28.65k forks source link

http: create response.overwriteHead() for things like stream piping error handling #52026

Open kwpartist opened 4 months ago

kwpartist commented 4 months ago

What is the problem this feature will solve?

Problem created after fix: #45508 to throw error on multiple writeHead calls (I personally updated to 20 LTS from 14 LTS recently).

This creates a new problem when wanting to pipe read streams to http response streams as you cannot overwrite the header response code upon a file read stream error event to reflect the file error (ENOENT, EISDIR, etc..) to give appropriate http responses (404, 500, etc.).

Example that will now break:

response.writeHead(200, {"Content-Type":"..."}); // HEADER STUFF
readStream.pipe(response);

readStream.on('error', function(error){
    if(error.code == 'ENOENT')
    {
        response.writeHead(404, {"Content-Type":"text/plain; charset=utf-8"});
        response.end('404 NOT FOUND', 'utf-8');
    }
    else if(error.code == 'EISDIR')
    {
        response.writeHead(403, {"Content-Type":"text/plain; charset=utf-8"});
        response.end('403 FORBIDDEN', 'utf-8');
    }
    else 
    {
        response.writeHead(500, {"Content-Type":"text/plain; charset=utf-8"});
        response.end('500 SERVER ERROR', 'utf-8');
    }
});

What is the feature you are proposing to solve the problem?

An identical function without the #45508 changes:

if (this._header) {
    throw new ERR_HTTP_HEADERS_SENT('write');
}

Proposing http response.overwriteHead() for at least 20 LTS +

Example:

response.writeHead(200, {"Content-Type":"..."}); // HEADER STUFF
readStream.pipe(response);

readStream.on('error', function(error){
    if(error.code == 'ENOENT')
    {
        response.overwriteHead(404, {"Content-Type":"text/plain; charset=utf-8"});
        response.end('404 NOT FOUND', 'utf-8');
    }
    else if(error.code == 'EISDIR')
    {
        response.overwriteHead(403, {"Content-Type":"text/plain; charset=utf-8"});
        response.end('403 FORBIDDEN', 'utf-8');
    }
    else 
    {
        response.overwriteHead(500, {"Content-Type":"text/plain; charset=utf-8"});
        response.end('500 SERVER ERROR', 'utf-8');
    }
});

What alternatives have you considered?

I would pull and go through the process of trying to add this feature myself if I knew the pulling process well. I'm new to GitHub I've always stayed solo in development so excuse my noob-ness. I understand it's much volunteered.

I've considered breaking apart the piping for fine grain control but that defeats the purpose of piping to begin with and for others unaware it may introduce memory leaks if they don't close both read and write streams on these kinds of error events properly.

(EDIT)

If calling writeHead multiple times is not detrimental to anything other than the effect of overwriting (which needs confirming), then perhaps the documentation could better reflect that instead of throwing an error on multiple use to simplify this solution. (Issue genesis: #36721)

Current docs: "This method must only be called once on a message and it must be called before response.end() is called..."

Re-worded to something like: "This function must be called before response.end() is called.

If response.write() or response.end() are called before this, the implicit/mutable headers will be calculated first.

This function will directly write the supplied header values onto the network channel and merge any headers supplied with response.setHeader() beforehand. Headers supplied to this function will have priority over response.setHeader() headers.

If response.setHeader() is never called before this, response.getHeader() will not yield the expected results as internal caching is bypassed. Use response.setHeader() if progressive population, future retrieval or modification is desired.

Caution multiple calls of this function on the same response, as these calls will overwrite the current header values and response code on the network channel..."

marco-ippolito commented 4 months ago

it seems a very specific use case, @ShogunPanda WDYT?

ShogunPanda commented 4 months ago

I'm not sure this method would make sense, IMHO. When you write writeHead and then start piping, the data starts flowing to the client. Overwriting the first line might not make sense as the response is already traveling through the socket.

Obviously, if the response is small enough it might happen that the response is still buffered and thus overwriting the headers make sense.

But for the general case I wouldn't assume this and therefore I'm -1 on this.

kwpartist commented 4 months ago

I understand what you're saying @ShogunPanda, it would only make sense if the errors are guaranteed to be thrown before the writable even has any data (ENOENT type errors for example). It wouldn't be for cases when data is already being piped after first chunk because the original set response code and headers wouldn't want to be changed anyway.

I feel a completely new method would be redundant for this specific use case, yes. That's why I feel better about the edited part of the OP---reversing the added throw because of docs confusion from #36721 and updating documentation to provide better clarity about how it works. It wasn't broken before, I used it in production in 14 LTS.

If not acceptable by the community the solution will be to create my own custom piper and inject the writeHead logic in-between the readable and writable---which requires handling the stream's other errors to prevent memory leaks (not a problem but added minutia).

I do this with uploads. I send auth data in the first chunks and pause the stream. If auth passes I resume the stream stripping away the auth data and proceed to create a temp file handle, etc., (otherwise it destroys the request as anything else would be malicious). This saves server resources very well.

ShogunPanda commented 4 months ago

Yes, I guess docs should be updated similarly as you propose. Would you like to send a PR?

kwpartist commented 4 months ago

I've yet to make any PRs (GitHub newbie), I read the relevant docs for this repo and it seems straightforward. Perhaps I give it a week or so if someone wishes to share the torch. Otherwise I will setup the git environment locally and make the contribution.

Much appreciated for the responses & feedback <3