feat: add 400 response for broken client request to instead of empty response #1829

XadillaX · 2017-12-14T02:11:56Z

Checklist

npm test passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

application

Description of change

Egg.js will do no response while client error. This PR makes server response a 400 Bad Request instead.

…response

fengmk2 · 2017-12-14T02:13:04Z

lib/application.js

 const WARN_CONFUSED_CONFIG = Symbol('Application#warnConfusedConfig');
 const EGG_LOADER = Symbol.for('egg#loader');
 const EGG_PATH = Symbol.for('egg#eggPath');
 const CLUSTER_CLIENTS = Symbol.for('egg#clusterClients');

+// client error => 400 Bad Request
+// Refs: https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_event_clienterror
+const DEFAULT_BAD_REQUEST_HTML = '<html>' +


还是这么顽固。。。

直接放到 400.html 然后 readFile 吧

@atian25 我没问题，看大家的一个决定。

这段是没办法走到 egg-onerror 那边的处理逻辑的是么？

这段是没办法走到 egg-onerror 那边的处理逻辑的是么？

语义上走不了。请求刚到，数据包还没解析完就要返回了，这个时候任何 resp 之类的 Wrapper 还没包上去，拿到的就是一个 raw 的 Socket 对象，你输出内容必须要输出一个 raw Response Packet。

fengmk2 · 2017-12-14T02:15:05Z

lib/application.js

@@ -55,6 +70,14 @@ class Application extends EggApplication {
    return path.join(__dirname, '..');
  }

+  [ON_CLIENT_ERROR](err, socket) {
+    this.logger.warn('A client error [%s] occurred: %s', err.code, err.message);


'[clientError] A client error [%s] occurred: %s'

然后记录下请求的 ip 和端口

方便排查问题

提了个 PR：nodejs/node#17672，不知道人家收不收。

如果收了的话，这个 logger 还能顺便将错误的 Packet Buffer 给输出来。

If this PR can be merged, we can output the raw packet buffer via logger.

fengmk2 · 2017-12-14T02:17:31Z

test/lib/cluster/app_worker.test.js

@@ -19,6 +19,41 @@ describe('test/lib/cluster/app_worker.test.js', () => {
      .expect('true');
  });

+  it('should response 400 bad request when HTTP request packet broken', () => {
+    const test = app.httpRequest()


连续发起请求2次，验证多次 clientError 也能够正确处理。

atian25 · 2017-12-14T02:26:54Z

test/lib/cluster/app_worker.test.js

@@ -19,6 +19,50 @@ describe('test/lib/cluster/app_worker.test.js', () => {
      .expect('true');
  });

+  it('should response 400 bad request when HTTP request packet broken', function* () {


改为 async 吧

codecov-io · 2017-12-14T02:33:17Z

Codecov Report

Merging #1829 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1829      +/-   ##
==========================================
+ Coverage   99.58%   99.59%   +<.01%     
==========================================
  Files          29       29              
  Lines         729      735       +6     
==========================================
+ Hits          726      732       +6     
  Misses          3        3

Impacted Files	Coverage Δ
lib/application.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0ee9f2...9362034. Read the comment docs.

dead-horse · 2017-12-14T03:07:04Z

lib/application.js

@@ -66,6 +93,8 @@ class Application extends EggApplication {
        this.coreLogger.error(err);
      },
    });
+
+    server.on('clientError', this[ON_CLIENT_ERROR].bind(this));


server.on('clientError', err => this[ON_CLIENT_ERROR](err)) 统一风格

另外我建议这里 onClientError 的 handler 不要用 symbol，给使用者一个覆盖的机会

有需要覆盖的需求再加吧，感觉这是一个兜底的，应该很少会自定义吧

@dead-horse 你指的是使用者在自己应用中继承这个类然后重载 onClientError 函数吗？

@atian25 如果我是使用者，我觉得我会想去自定义

这种路径不是正常用户能触发的吧

@atian25 这只是一种情况，clientError 可不止这一种情况。举个最简单的例子，用户的请求被运营商劫持了，然后运营商代码有误，把请求包改错了。

使用者在自己应用中继承这个类然后重载 onClientError 函数吗

使用者直接在 extends/app.js 中就可以覆盖默认的 onClientError 实现了

XadillaX · 2017-12-14T04:54:31Z

lib/application.js

@@ -55,6 +69,18 @@ class Application extends EggApplication {
    return path.join(__dirname, '..');
  }

+  onClientError(err, socket) {
+    this.logger.error('A client (%s:%d) error [%s] occurred: %s',


提了个 PR：nodejs/node#17672

不知道人家收不收。

如果收了的话，这个 logger 还能顺便将错误的 Packet Buffer 给输出来。

If this PR can be merged, we can output the raw packet buffer via logger.

dead-horse · 2017-12-15T05:26:17Z

@XadillaX pick 到 1.x 分支吧

feat: add 400 response for broken client request to instead of empty response (#1829)

XadillaX · 2017-12-15T06:05:05Z

@dead-horse 我来操作吗？

atian25 · 2017-12-15T06:16:26Z

嗯

…response (eggjs#1829)

fengmk2 · 2023-10-14T02:17:01Z

强如死月！

feat: add 400 response for broken client request to instead of empty …

9d2a647

…response

XadillaX changed the title ~~feat: add 400 response for broken client request to instead of empty …~~ feat: add 400 response for broken client request to instead of empty response Dec 14, 2017

fengmk2 reviewed Dec 14, 2017

View reviewed changes

fengmk2 added the core: application label Dec 14, 2017

XadillaX added 2 commits December 14, 2017 10:20

r

98dc445

r

00568d8

atian25 reviewed Dec 14, 2017

View reviewed changes

r

9aac3cc

dead-horse reviewed Dec 14, 2017

View reviewed changes

XadillaX added 2 commits December 14, 2017 11:13

r

ebbd6ab

r

9362034

dead-horse approved these changes Dec 14, 2017

View reviewed changes

XadillaX commented Dec 14, 2017

View reviewed changes

dead-horse merged commit 40df153 into eggjs:master Dec 15, 2017

popomore pushed a commit that referenced this pull request Dec 15, 2017

docs: auto generate by ci

c9f1990

feat: add 400 response for broken client request to instead of empty response (#1829)

XadillaX added a commit to NBUT-Developers/egg that referenced this pull request Dec 15, 2017

feat: add 400 response for broken client request to instead of empty …

a883296

…response (eggjs#1829)

This was referenced Dec 15, 2017

feat: add 400 response for broken client request to instead of empty response #1836

Merged

handle clientError #1516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add 400 response for broken client request to instead of empty response #1829

feat: add 400 response for broken client request to instead of empty response #1829

XadillaX commented Dec 14, 2017 •

edited

Loading

fengmk2 Dec 14, 2017

atian25 Dec 14, 2017

XadillaX Dec 14, 2017

atian25 Dec 14, 2017

XadillaX Dec 14, 2017

fengmk2 Dec 14, 2017

fengmk2 Dec 14, 2017

XadillaX Dec 14, 2017 •

edited

Loading

fengmk2 Dec 14, 2017

atian25 Dec 14, 2017

codecov-io commented Dec 14, 2017 •

edited

Loading

dead-horse Dec 14, 2017

dead-horse Dec 14, 2017

atian25 Dec 14, 2017

XadillaX Dec 14, 2017

XadillaX Dec 14, 2017

atian25 Dec 14, 2017

XadillaX Dec 14, 2017 •

edited

Loading

dead-horse Dec 14, 2017

XadillaX Dec 14, 2017

dead-horse commented Dec 15, 2017

XadillaX commented Dec 15, 2017

atian25 commented Dec 15, 2017

fengmk2 commented Oct 14, 2023

feat: add 400 response for broken client request to instead of empty response #1829

feat: add 400 response for broken client request to instead of empty response #1829

Conversation

XadillaX commented Dec 14, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 14, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dead-horse commented Dec 15, 2017

XadillaX commented Dec 15, 2017

atian25 commented Dec 15, 2017

fengmk2 commented Oct 14, 2023

XadillaX commented Dec 14, 2017 •

edited

Loading

XadillaX Dec 14, 2017 •

edited

Loading

codecov-io commented Dec 14, 2017 •

edited

Loading

XadillaX Dec 14, 2017 •

edited

Loading