Skip to content

fix grpc bugs#40

Merged
gxcsoccer merged 4 commits intosofastack:masterfrom
smile21:master
Mar 26, 2019
Merged

fix grpc bugs#40
gxcsoccer merged 4 commits intosofastack:masterfrom
smile21:master

Conversation

@smile21
Copy link
Copy Markdown
Contributor

@smile21 smile21 commented Mar 19, 2019

  1. 修复 grpc invoke 多次但返回数据相同问题
  2. 修复grpc server service 用户逻辑抛异常,但调用方长时间无响应问题

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 19, 2019

Codecov Report

Merging #40 into master will decrease coverage by 0.09%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #40     +/-   ##
=========================================
- Coverage   98.55%   98.46%   -0.1%     
=========================================
  Files          38       38             
  Lines        1871     1890     +19     
=========================================
+ Hits         1844     1861     +17     
- Misses         27       29      +2
Impacted Files Coverage Δ
lib/client/connection/grpc/call_stream.js 92.12% <100%> (-0.57%) ⬇️
lib/server/service.js 100% <100%> (ø) ⬆️
lib/server/server.js 100% <100%> (ø) ⬆️
lib/server/grpc/response.js 97.5% <95.23%> (-2.5%) ⬇️

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 b5e2a84...80cec3a. Read the comment docs.

Comment thread lib/server/grpc/response.js Outdated
const responseType = methodInfo.resolvedResponseType;
const buf = responseType.encode(responseType.fromObject(res.appResponse)).finish();
const resSize = buf.length;
io.position(0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

io.reset()

Comment thread lib/server/grpc/response.js Outdated
this.stream.close(http2.constants.NGHTTP2_INTERNAL_ERROR);
}
return;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这里没必要断开吧

Comment thread lib/server/server.js Outdated
appResponse: null,
});
throw new Error('not found service: ' + id);
} else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

不用 else 了

@gxcsoccer gxcsoccer self-requested a review March 25, 2019 15:32
@gxcsoccer
Copy link
Copy Markdown
Member

感谢 PR

}
this._endCall(err);
this.destroy();
this.emit('close');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

这个应该不用主动触发吧? destroy 以后会自动触发 close

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

是的 我的 node 是 v8.x ,在这个版本里 destroy 后收不到 close 事件,所以我手动 emit 了一个

Copy link
Copy Markdown
Contributor Author

@smile21 smile21 Mar 26, 2019

Choose a reason for hiding this comment

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

可能和这个 pr 有关 nodejs/node#23654 v8.x 目前还是有这个问题

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

加一个注释?

};
let data = '';
if (res.isError) {
this.meta.rt = Date.now() - this.meta.start;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

rt 可以统一设置

@gxcsoccer gxcsoccer merged commit 2439e1e into sofastack:master Mar 26, 2019
@gxcsoccer
Copy link
Copy Markdown
Member

sofa-rpc-node@1.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants