-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: only set keep-alive header before Node.js 12.19.0 #4491
Conversation
这样肯定不行啦,12 和 14 之间的就有问题了。 我指的是给 Node 12 那边提 PR |
没太看懂啥意思?Node那边应该是因为12.19.0是LTS版本,所以合并了。 |
Codecov Report
@@ Coverage Diff @@
## master #4491 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 34 34
Lines 1113 1113
Branches 183 183
=========================================
Hits 1113 1113
Continue to review full report at Codecov.
|
这个问题的最根本原因是, Node 那边没有判断 keep-alive 是否存在,重复设置了,所以应该是给 Node 那边提 PR 修复,你看我有提了一个到 Node 14 了。 你的意思是 Node 12 也 backport 了他们之前有问题的 PR,导致也出问题了?如果是的话,那可以去 backport 我的那个 PR 让 12 也不会重复设置。 |
我看 CI 的 12 跑是没问题的,你确认最新的 Node 12 会导致重复设置? |
travis-ci上12会有重复设置的问题 |
好像是,那需要把那个 PR backport 给 Node 12 |
@qingdengyue 可以把 nodejs/node#35138 backport 到 Node.js v12.x,按照这个文档来:https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md |
@qingdengyue https://github.com/eggjs/egg/pull/4497/files 要这样判断区间,而不是直接改 |
Checklist
npm test
passesAffected core subsystem(s)
Description of change
only set keep-alive header before Node.js 12.19.0