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: response.setHeaders() #46109

Merged
merged 12 commits into from
Jan 9, 2023

Conversation

marco-ippolito
Copy link
Member

from: #46082
I've extracted from res.writeHead the part where it sets multiple headers into res.setHeaders.
I've removed this part in writeHead:
if (k === undefined && this._header) { throw new ERR_HTTP_HEADERS_SENT('render'); }
because since #45508 it never enters that condition.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jan 5, 2023
doc/api/http.md Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
lib/_http_server.js Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jan 6, 2023

One problem that emerged is that I cannot check if (headers instanceof Headers) because importing Headers creates a circular dependency with undici. I need to check it in another way. Maybe if headers has a keys and get function, this would also allow Maps

Co-authored-by: Rich Trott <rtrott@gmail.com>
@ShogunPanda
Copy link
Contributor

One problem that emerged is that I cannot check if (headers instanceof Headers) because importing Headers creates a circular dependency with undici. I need to check it in another way. Maybe if headers has a keys and get function, this would also allow Maps

+1 on this. I would also outline in the docs somehow.

lib/_http_outgoing.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

Little bit. Rest looks fine.

doc/api/http.md Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
@jimmywarting
Copy link

One problem that emerged is that I cannot check if (headers instanceof Headers) because importing Headers creates a circular dependency with undici. I need to check it in another way. Maybe if headers has a keys and get function, this would also allow Maps

Hmm confused as to why you can't import Headers 😕
Don't know how NodeJS/undici creates a circular dependency. (haven't really dug into the code. But if that's they way it got to be then OK.

Maybe that was the reason why you could not do something similar to:

res.setHeaders = function (headers) {
  if (headers instanceof Headers === false) headers = new Headers(headers)
  if (headers[symbol.toStringTag] !== 'Headers') headers = new Headers(headers) // or this
  
  for (const [key, value] of headers) {
    this.setHeader(key, value)
  }
}

?

@marco-ippolito
Copy link
Member Author

@jimmywarting it created a circular dependency because Headers is export by Undici which requires the http module. (the globalThis.Headers is forbidden)
Since Headers is basically an extension of Map I think it was a good idea to allow Map.

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
PR-URL: nodejs#46109
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@RafaelGSS
Copy link
Member

@marco-ippolito this didn't land cleanly on v19.x-staging because you are removing ERR_HTTP_HEADERS_SENT without removing it from the top (the v19.x doesn't include the #45508).

I'll open a backport for you.

@RafaelGSS
Copy link
Member

Actually, I can't. Your test relies on a feature available only on main requireHostHeader (semver-major). Can you create a manual backport?

marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Jan 19, 2023
PR-URL: nodejs#46109
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@juanarbol juanarbol added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jan 25, 2023
@juanarbol
Copy link
Member

This is breaking test/parallel/test-http-write-head.js in v18.x

/usr/local/opt/python@3.10/bin/python3.10 tools/test.py  --mode=release \                                                                                                                      
                 \                                                                                                                                                                             
                --skip-tests= \                                                                                                                                                                
                default \                                                                                                                                                                      
                addons js-native-api node-api                                                                                                                                                  
=== release test-http-write-head ===                                                                                                                                                           
Path: parallel/test-http-write-head                                                                                                                                                            
node:assert:124                                                                                                                                                                                
  throw new AssertionError(obj);                                                                                                                                                               
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception (Error).
    at Server.<anonymous> (/Users/juan/GitHub/node/test/parallel/test-http-write-head.js:56:10) 
    at Server.<anonymous> (/Users/juan/GitHub/node/test/common/index.js:446:15)
    at Server.emit (node:events:513:28)
    at parserOnIncoming (node:_http_server:1069:12)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: {
    code: 'ERR_HTTP_HEADERS_SENT',
    name: 'Error',
    message: 'Cannot render headers after they are sent to the client'
  },
  operator: 'throws'
}

@marco-ippolito
Copy link
Member Author

I have created a manual backport to 19.x #46272, should I create another for 18.x?

@juanarbol
Copy link
Member

I have created a manual backport to 19.x #46272, should I create another for 18.x?

It would great! Thanks!

marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Jan 26, 2023
PR-URL: nodejs#46109
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
ruyadorno pushed a commit that referenced this pull request Jan 31, 2023
Backport-PR-URL: #46272
PR-URL: #46109
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@ruyadorno ruyadorno mentioned this pull request Feb 1, 2023
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Feb 21, 2023
PR-URL: nodejs#46109
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Mar 1, 2023
PR-URL: nodejs#46109
Backport-PR-URL: nodejs#46365
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
PR-URL: #46109
Backport-PR-URL: #46365
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.