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

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

Closed
kwpartist opened this issue Mar 9, 2024 · 7 comments
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. stale

Comments

@kwpartist
Copy link

kwpartist commented Mar 9, 2024

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..."

@kwpartist kwpartist added the feature request Issues that request new features to be added to Node.js. label Mar 9, 2024
@github-project-automation github-project-automation bot moved this to Pending Triage in Node.js feature requests Mar 9, 2024
@marco-ippolito marco-ippolito added the http Issues or PRs related to the http subsystem. label Mar 11, 2024
@marco-ippolito
Copy link
Member

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

@ShogunPanda
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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

@kwpartist
Copy link
Author

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

Copy link
Contributor

github-actions bot commented Sep 9, 2024

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 9, 2024
Copy link
Contributor

github-actions bot commented Oct 9, 2024

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. stale
Projects
None yet
Development

No branches or pull requests

3 participants