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

send response之后,request/response对象析构之前执行一些自定义逻辑 #2328

Merged
merged 10 commits into from
Oct 25, 2023

Conversation

yockie
Copy link
Contributor

@yockie yockie commented Jul 25, 2023

What problem does this PR solve?

1.我们的场景希望将响应时间减少到最小,所以希望将一些类似数据上报的耗时操作(是的,特别耗时)放在response返回之后、request/response析构之前做
2.本pr借鉴了 #1433 ,看这个pr一直没合入且测试发现有些问题&不太满足我们的需要,所以另提一个。 #1433 的问题主要是:
(1)太多无关topic的改动
(2)仅在SendRpcResponse成功时才调用call_after_rpc_resp函数,失败时没有调用(我们需要无论成功或失败都要调用)
(3)定义的after_resp_fn_t不包含controller指针,我们需要拿到controller对象(进而拿到session data指针,所以声明为非const指针)
3.同样在目前在常用的 baidu_std/http/http2 协议上支持。其他协议未开发。
4.将Controller与req、res的析构顺序进行了修改,依赖关系应该是controller依赖req、res,所以应该先析构controller,后析构req、res

麻烦maintainers看看有什么不妥之处~

@yockie yockie changed the title response返回之后,request/response析构之前执行一些自定义逻辑 send response之后,request/response析构之前执行一些自定义逻辑 Jul 25, 2023
@yockie yockie changed the title send response之后,request/response析构之前执行一些自定义逻辑 send response之后,request/response对象析构之前执行一些自定义逻辑 Jul 25, 2023
src/brpc/controller.h Outdated Show resolved Hide resolved
@wwbmmm
Copy link
Contributor

wwbmmm commented Jul 26, 2023

另外,补充一下单测吧

@yockie
Copy link
Contributor Author

yockie commented Jul 26, 2023

另外,补充一下单测吧

done

@yockie
Copy link
Contributor Author

yockie commented Jul 27, 2023

另外,补充一下单测吧

之前单测写的不对,已经修改了,本地跑通了

@yockie yockie requested a review from wwbmmm July 27, 2023 14:39
src/brpc/controller.h Outdated Show resolved Hide resolved
@wwbmmm
Copy link
Contributor

wwbmmm commented Aug 16, 2023

LGTM

@wwbmmm wwbmmm merged commit e1a8863 into apache:master Oct 25, 2023
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