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

refactor: add read-write lock to ExtensionContextRegistry #4245

Merged
merged 2 commits into from Jul 21, 2023

Conversation

guqing
Copy link
Member

@guqing guqing commented Jul 17, 2023

What type of PR is this?

/kind improvement
/area core
/area plugin
/milestone 2.8.x

What this PR does / why we need it:

修复由于多线程环境下导致的插件卸载时的路由异常问题

改动描述:
为了确保在多线程环境下访问 ExtensionContextRegistry 类的注册表时的线程安全。通过添加读写锁,可以保证在读取和写入PluginApplicationContext 时只有一个线程可以访问,从而避免了多个线程同时访问注册表时可能出现的竞态条件和数据不一致的问题。同时,更新了 register、remove、getByPluginId、containsContext 和 getPluginApplicationContexts 方法,以在访问注册表时获取和释放适当的锁,从而确保了线程安全。

问题原因:
当插件卸载时,卸载动作在 Reconciler 线程中执行而路由访问是在 reactor 的 NonBlockingThread 线程执行,当 PluginCompositeRouterFunction 的 routerFunctions() 方法从 ExtensionContextRegistry 中获取所有 PluginApplicationContext 并持有还未处理完成时由于 PluginReconciler 中执行了卸载插件逻辑而将某个 PluginApplicationContext 关闭从而让 PluginCompositeRouterFunction 中持有到的对象引用发生变化出现数据不一致问题导致出现 PluginApplicationContext@14971c8e has been closed already 异常。

解决方案:
所以此修改让读取和写入PluginApplicationContext 时只有一个线程可以访问来解决此问题

how to test it?
测试开发模式下卸载插件时是否会出现如 #4242 中所描述的异常信息

Which issue(s) this PR fixes:

Fixes #4242

Does this PR introduce a user-facing change?

修复由于多线程环境下导致的插件卸载时的路由异常问题

@f2c-ci-robot f2c-ci-robot bot added kind/improvement Categorizes issue or PR as related to a improvement. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 17, 2023
@f2c-ci-robot f2c-ci-robot bot added this to the 2.8.x milestone Jul 17, 2023
@f2c-ci-robot f2c-ci-robot bot added area/core Issues or PRs related to the Halo Core area/plugin Issues or PRs related to the Plugin Provider labels Jul 17, 2023
@guqing
Copy link
Member Author

guqing commented Jul 17, 2023

/hold 等待 #4241 合并避免重复测试

@f2c-ci-robot f2c-ci-robot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2023
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #4245 (8b6f8e5) into main (5a0e202) will decrease coverage by 0.07%.
The diff coverage is 66.10%.

@@             Coverage Diff              @@
##               main    #4245      +/-   ##
============================================
- Coverage     60.14%   60.07%   -0.07%     
+ Complexity     2413     2411       -2     
============================================
  Files           366      366              
  Lines         12587    12610      +23     
  Branches        906      906              
============================================
+ Hits           7570     7576       +6     
- Misses         4578     4594      +16     
- Partials        439      440       +1     
Impacted Files Coverage Δ
...plugin/PluginApplicationEventBridgeDispatcher.java 21.42% <0.00%> (-3.58%) ⬇️
.../halo/app/plugin/PluginApplicationInitializer.java 8.73% <0.00%> (+0.13%) ⬆️
.../run/halo/app/plugin/ExtensionContextRegistry.java 66.66% <59.25%> (+6.66%) ⬆️
...halo/app/plugin/PluginCompositeRouterFunction.java 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

/lgtm

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2023
@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Jul 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnNiang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2023
Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

/unhold

@f2c-ci-robot f2c-ci-robot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2023
@f2c-ci-robot f2c-ci-robot bot merged commit 3b03ed9 into halo-dev:main Jul 21, 2023
4 checks passed
@guqing guqing deleted the refactor/4242 branch July 21, 2023 04:25
@ruibaby ruibaby modified the milestones: 2.8.x, 2.8.0 Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/core Issues or PRs related to the Halo Core area/plugin Issues or PRs related to the Plugin Provider kind/improvement Categorizes issue or PR as related to a improvement. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

插件被卸载时插件 Router 会抛出异常
3 participants