Skip to content

fix additional refrerence can not release when socket is reviving#1817

Merged
zyearn merged 5 commits into
apache:masterfrom
chenBright:fix_socket_recyle_flag
Jul 25, 2022
Merged

fix additional refrerence can not release when socket is reviving#1817
zyearn merged 5 commits into
apache:masterfrom
chenBright:fix_socket_recyle_flag

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

fix #1773

#1774 的pr是使用了加锁的方案,该pr实现了@wwbmmm#1774 提到的方案

_recycle_flag增加reviving状态。如果ReleaseAddtionalReference发现状态是reviving就一直等,直到遇到其它两个状态再继续释放additional refrerence。

Comment thread src/brpc/socket.cpp Outdated
Comment thread src/brpc/socket.cpp Outdated
@wwbmmm

wwbmmm commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

LGTM

@chenBright

Copy link
Copy Markdown
Contributor Author

@wwbmmm 大概什么时候能合呢?

@zyearn zyearn self-assigned this Jul 1, 2022
@chenBright

chenBright commented Jul 18, 2022

Copy link
Copy Markdown
Contributor Author

@zyearn 麻烦看看这个pr。

@zyearn

zyearn commented Jul 19, 2022

Copy link
Copy Markdown
Member

@chenBright 我这两天看一下。

@zyearn

zyearn commented Jul 21, 2022

Copy link
Copy Markdown
Member

一个Nice To Have:这里会有2次HC,第一次HC是真正的HC,第二次HC是因为这个极端case导致的false positive(其实对端server已经恢复了),理论上只做一次HC即可。

可以不用在这个PR实现,有兴趣的话可以想想怎么解决 :)

Comment thread src/brpc/socket.h Outdated
Comment thread src/brpc/socket.cpp
@chenBright

Copy link
Copy Markdown
Contributor Author

一个Nice To Have:这里会有2次HC,第一次HC是真正的HC,第二次HC是因为这个极端case导致的false positive(其实对端server已经恢复了),理论上只做一次HC即可。

可以不用在这个PR实现,有兴趣的话可以想想怎么解决 :)

设个标志_is_hc_started(atomic),默认为false。hc开始的时候判断_is_hc_started.compare_exchange_strong(expect, true,butil::memory_order_relaxed),其中expect为false。如果为true,则表示无hc,可以进行hc;否则,跳过,不进行hc。当hc结束时,将_is_hc_started置为false。这样应该可以避免同时进行2次hc的问题。

@chenBright chenBright force-pushed the fix_socket_recyle_flag branch 2 times, most recently from bfc1621 to 485bbf8 Compare July 23, 2022 10:39
@chenBright chenBright force-pushed the fix_socket_recyle_flag branch from 485bbf8 to ec32816 Compare July 23, 2022 15:07
@zyearn

zyearn commented Jul 23, 2022

Copy link
Copy Markdown
Member

一个Nice To Have:这里会有2次HC,第一次HC是真正的HC,第二次HC是因为这个极端case导致的false positive(其实对端server已经恢复了),理论上只做一次HC即可。
可以不用在这个PR实现,有兴趣的话可以想想怎么解决 :)

设个标志_is_hc_started(atomic),默认为false。hc开始的时候判断_is_hc_started.compare_exchange_strong(expect, true,butil::memory_order_relaxed),其中expect为false。如果为true,则表示无hc,可以进行hc;否则,跳过,不进行hc。当hc结束时,将_is_hc_started置为false。这样应该可以避免同时进行2次hc的问题。

Another Nice-to-have: 有一种情况会导致2次连续(不并发)的HC:SetFailed后调度出去,另外一个线程的HC结束,然后SetFailed继续执行到StartHealthCheck。当然第二次是必要的因为第一次HC之后如果server立刻down了,这第二次HC可以检测出来,不过这个场景大概率第二次HC是成功的,因为之前紧接的第一次是成功的。

我们还可以继续优化掉这个第二次HC吗?如果发生了server在第一次HC后立刻down的情况,可以在下一次用户主动的rpc call的时候再触发HC.

@chenBright

Copy link
Copy Markdown
Contributor Author

一个Nice To Have:这里会有2次HC,第一次HC是真正的HC,第二次HC是因为这个极端case导致的false positive(其实对端server已经恢复了),理论上只做一次HC即可。
可以不用在这个PR实现,有兴趣的话可以想想怎么解决 :)

设个标志_is_hc_started(atomic),默认为false。hc开始的时候判断_is_hc_started.compare_exchange_strong(expect, true,butil::memory_order_relaxed),其中expect为false。如果为true,则表示无hc,可以进行hc;否则,跳过,不进行hc。当hc结束时,将_is_hc_started置为false。这样应该可以避免同时进行2次hc的问题。

Another Nice-to-have: 有一种情况会导致2次连续(不并发)的HC:SetFailed后调度出去,另外一个线程的HC结束,然后SetFailed继续执行到StartHealthCheck。当然第二次是必要的因为第一次HC之后如果server立刻down了,这第二次HC可以检测出来,不过这个场景大概率第二次HC是成功的,因为之前紧接的第一次是成功的。

我们还可以继续优化掉这个第二次HC吗?如果发生了server在第一次HC后立刻down的情况,可以在下一次用户主动的rpc call的时候再触发HC.

这种情况好像没有好的优化方法,此场景下的第二次SetFailed和普通场景下的SetFailed差不多。_versioned_ref的恢复需要HC,如果发生了server在第一次HC后立刻down的情况,不在这次触发HC的话,_versioned_ref就无法恢复,用户也无法address到socket,主动rpc call了。

Comment thread src/brpc/socket.h Outdated
Comment thread src/brpc/socket.h Outdated
@zyearn

zyearn commented Jul 24, 2022

Copy link
Copy Markdown
Member

一个Nice To Have:这里会有2次HC,第一次HC是真正的HC,第二次HC是因为这个极端case导致的false positive(其实对端server已经恢复了),理论上只做一次HC即可。
可以不用在这个PR实现,有兴趣的话可以想想怎么解决 :)

设个标志_is_hc_started(atomic),默认为false。hc开始的时候判断_is_hc_started.compare_exchange_strong(expect, true,butil::memory_order_relaxed),其中expect为false。如果为true,则表示无hc,可以进行hc;否则,跳过,不进行hc。当hc结束时,将_is_hc_started置为false。这样应该可以避免同时进行2次hc的问题。

Another Nice-to-have: 有一种情况会导致2次连续(不并发)的HC:SetFailed后调度出去,另外一个线程的HC结束,然后SetFailed继续执行到StartHealthCheck。当然第二次是必要的因为第一次HC之后如果server立刻down了,这第二次HC可以检测出来,不过这个场景大概率第二次HC是成功的,因为之前紧接的第一次是成功的。
我们还可以继续优化掉这个第二次HC吗?如果发生了server在第一次HC后立刻down的情况,可以在下一次用户主动的rpc call的时候再触发HC.

这种情况好像没有好的优化方法,此场景下的第二次SetFailed和普通场景下的SetFailed差不多。_versioned_ref的恢复需要HC,如果发生了server在第一次HC后立刻down的情况,不在这次触发HC的话,_versioned_ref就无法恢复,用户也无法address到socket,主动rpc call了。

是的,这是这个优化在实现上比较难的地方,先暂时放一放吧。

@zyearn zyearn merged commit 83fc7b7 into apache:master Jul 25, 2022
@zyearn

zyearn commented Jul 25, 2022

Copy link
Copy Markdown
Member

感谢贡献

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.

brpc socket引用计数极端场景下少减了一次,导致socket无法建立新连接

3 participants