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

fix: get rid of unsafe code by arc-swap #234

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

thynson
Copy link
Contributor

@thynson thynson commented Jul 9, 2024

No description provided.

@CherishCai
Copy link
Collaborator

pls cargo fmt --all format code

@CherishCai
Copy link
Collaborator

先前用 unsafe 而不用 RWLock 是因为在单线程下会死锁,tokio::test 测试用例之下一直卡住无法拿到登录 token 。
所以这个修改请确保单线程测试用例能跑通登录验证。

@thynson
Copy link
Contributor Author

thynson commented Jul 10, 2024

先前用 unsafe 而不用 RWLock 是因为在单线程下会死锁,tokio::test 测试用例之下一直卡住无法拿到登录 token 。 所以这个修改请确保单线程测试用例能跑通登录验证。

ArcSwap 提供的是原子替换 Arc 对象的能力,因而可以在不使用锁的前提下提供内部可变性,也就不会有死锁的问题。本文件内的测试是通过了的。

Copy link
Collaborator

@CherishCai CherishCai left a comment

Choose a reason for hiding this comment

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

LGTM

@thynson
Copy link
Contributor Author

thynson commented Jul 10, 2024

实际上最好的改法可能还是把改成需要&mut self,这样一来不会存在同时同一毫秒内多个login触发多个请求的问题(因为所有请求实际上被序列化了),二来也不需要通过ArcSwap来处理状态,只是这么改牵动太大。

@CherishCai CherishCai merged commit 40bef73 into nacos-group:main Jul 10, 2024
5 checks passed
@CherishCai
Copy link
Collaborator

实际上最好的改法可能还是把改成需要&mut self,这样一来不会存在同时同一毫秒内多个login触发多个请求的问题(因为所有请求实际上被序列化了),二来也不需要通过ArcSwap来处理状态,只是这么改牵动太大。

未来有强烈必要则再改,毕竟需要自定义 login 的并不多,需要的同学相信他们也能理解

@CherishCai
Copy link
Collaborator

CherishCai commented Jul 15, 2024

实际上最好的改法可能还是把改成需要&mut self,这样一来不会存在同时同一毫秒内多个login触发多个请求的问题(因为所有请求实际上被序列化了),二来也不需要通过ArcSwap来处理状态,只是这么改牵动太大。

@thynson 在 Send + Sync 的 async 环境下,改为 &mut self 的 AuthPlugin 是否不太可行?跨线程修改那也得同样有锁

@thynson
Copy link
Contributor Author

thynson commented Jul 16, 2024

看了下确实不太好改,还有一个是在Arc<dyn Trait>里使用了,导致也必须用不可变引用,先保持这个样子吧。

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