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.

Sign up for GitHub

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: 当config.isExpireAfterAccess() 为false时，无需加锁 #952

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: 当config.isExpireAfterAccess() 为false时，无需加锁 #952

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions jetcache-core/src/main/java/com/alicp/jetcache/embedded/AbstractEmbeddedCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,19 +58,19 @@ protected CacheGetResult<V> parseHolderResult(CacheValueHolder<V> holder) {
} else if (now >= holder.getExpireTime()) {
return CacheGetResult.EXPIRED_WITHOUT_MSG;
} else {
lock.lock();
try{
long accessTime = holder.getAccessTime();
if (config.isExpireAfterAccess()) {
if (config.isExpireAfterAccess()) {
lock.lock();
try {
long accessTime = holder.getAccessTime();
long expireAfterAccess = config.getExpireAfterAccessInMillis();
if (now >= accessTime + expireAfterAccess) {
return CacheGetResult.EXPIRED_WITHOUT_MSG;
}
} finally {
lock.unlock();
}
holder.setAccessTime(now);
}finally {
lock.unlock();
}
holder.setAccessTime(now);
Copy link
Collaborator

Choose a reason for hiding this comment

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

这一行得进锁里面去。

Copy link
Author

@WangFeiCoder WangFeiCoder Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

放进锁的目的为为了什么呢，当config.isExpireAfterAccess() 为false时，判断逻辑不会走到，加一个锁 仅为了赋值吗。代码中锁加在else 并没没有整个方法，应不会影响其他获取holder对象属性吧。或者是有地方获取holder时加了锁？

Copy link
Collaborator

@areyouok areyouok Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

对accessTime的读和写必须都在锁内，如果放在锁外，那么对accessTime的写入操作不能保证被别的线程看见。

此外，如果config里面的expireAfterAccess和expireAfterAccessInMillis需要在运行时间动态调整的话，光使用这里这个锁是不管用的，只能实现不严格的最终可见，目前就只打算做到这样。

Copy link
Collaborator

Choose a reason for hiding this comment

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

其实，要增加效率，这个锁就应该去掉，把这些内容放到子类LinkedHashMapCache/CaffeineCache里面去处理，就不用额外加锁了，但是要改涉及到的东西也有点多（放在AbstractEmbeddedCache里面当时主要是想在父类里面统一处理一些逻辑，既然这个逻辑下沉到子类的，其它相关逻辑是不是也要下沉到子类），由于当前定义的InnerMap不能返回错误码，只能用返回null代替，那么最终返回的EXPIRED就会变成NOT_EXISTS，行为略有改变，一些单元测试可能会失败。或者，就要修改InnerMap的定义。


return new CacheGetResult(CacheResultCode.SUCCESS, null, holder);
}
Expand Down