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

Imp: destroy invoker smoothly #1045

Merged
merged 3 commits into from
Feb 22, 2021
Merged

Imp: destroy invoker smoothly #1045

merged 3 commits into from
Feb 22, 2021

Conversation

AlexStocks
Copy link
Contributor

What this PR does:
fix bug: protocol/dubbo/dubbo_invoker.go:DubboInvoker.reqNum should be a atomic number

@AlexStocks AlexStocks changed the base branch from master to 1.5 February 5, 2021 15:41
Copy link
Member

@wenxuwan wenxuwan left a comment

Choose a reason for hiding this comment

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

这里使用原子操作和前面的没什么区别,还是建议使用读写锁来做

@gaoxinge

This comment has been minimized.

@AlexStocks AlexStocks changed the title Fix: DubboInvoker.reqnum should be atomic Imp: destroy invoker smoothly Feb 6, 2021
@codecov-io
Copy link

codecov-io commented Feb 6, 2021

Codecov Report

Merging #1045 (d84ece5) into 1.5 (968650f) will decrease coverage by 0.22%.
The diff coverage is 53.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.5    #1045      +/-   ##
==========================================
- Coverage   59.53%   59.31%   -0.23%     
==========================================
  Files         259      265       +6     
  Lines       12737    13137     +400     
==========================================
+ Hits         7583     7792     +209     
- Misses       4199     4362     +163     
- Partials      955      983      +28     
Impacted Files Coverage Δ
cluster/cluster_impl/available_cluster.go 100.00% <ø> (ø)
cluster/router/condition/listenable_router.go 57.89% <0.00%> (+3.17%) ⬆️
cluster/router/healthcheck/health_check_route.go 71.42% <0.00%> (+1.05%) ⬆️
cluster/router/tag/router_rule.go 89.47% <ø> (+12.20%) ⬆️
cluster/router/tag/tag_router.go 72.99% <ø> (+0.40%) ⬆️
common/config/environment.go 51.72% <ø> (ø)
common/extension/config_post_processor.go 0.00% <0.00%> (ø)
common/extension/metadata_service.go 0.00% <0.00%> (ø)
common/proxy/proxy_factory/default.go 13.55% <0.00%> (+0.22%) ⬆️
config/base_config.go 64.02% <ø> (ø)
... and 166 more

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 7d0b63a...d84ece5. Read the comment docs.

protocol/dubbo/dubbo_invoker.go Outdated Show resolved Hide resolved
protocol/dubbo/dubbo_invoker.go Outdated Show resolved Hide resolved
protocol/grpc/grpc_invoker.go Outdated Show resolved Hide resolved
protocol/dubbo/dubbo_invoker.go Show resolved Hide resolved
protocol/dubbo/dubbo_invoker.go Outdated Show resolved Hide resolved
}
break
} else if times < 0 {
Copy link
Member

Choose a reason for hiding this comment

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

This check not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

万一发生什么幺蛾子呢?防御性编程,这里面有日志的。

protocol/dubbo/dubbo_invoker.go Show resolved Hide resolved
protocol/dubbo/dubbo_invoker.go Outdated Show resolved Hide resolved
protocol/grpc/grpc_invoker.go Outdated Show resolved Hide resolved
@wenxuwan
Copy link
Member

wenxuwan commented Feb 8, 2021

这边可以用给一个原子操作代表invoker是不是可用状态,然后加一个读写锁,invoke的时候先用原子操作判断是不是可用的,如果可用加读锁,然后再通过原子操作判断一次,如果还可用,调用下面的call,完成后释放读锁。destroy的时候直接把invoker标记为不可用状态,然后阻塞获取写锁,获取成功后然后关闭invoker,释放client就可以了。

Copy link
Member

@flycash flycash left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexStocks
Copy link
Contributor Author

@flycash pls check it again. I have reconstructed this pr as @wenxuwan's suggestion.

@flycash
Copy link
Member

flycash commented Feb 10, 2021

@flycash pls check it again. I have reconstructed this pr as @wenxuwan's suggestion.

LGTM

@zouyx zouyx added the improve Refactor or improve label Feb 19, 2021
@AlexStocks AlexStocks merged commit 7d52dd1 into 1.5 Feb 22, 2021
@cityiron cityiron added this to the v1.5.7 milestone Feb 25, 2021
AlexStocks added a commit that referenced this pull request Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve Refactor or improve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants