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

lock.lock() before try #7582

Closed
wants to merge 0 commits into from
Closed

lock.lock() before try #7582

wants to merge 0 commits into from

Conversation

shenjianeng
Copy link

lock.lock() before try

Copy link
Contributor

@joeCarf joeCarf left a comment

Choose a reason for hiding this comment

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

looks good

@shenjianeng shenjianeng requested a review from joeCarf November 26, 2023 13:07
Copy link
Member

@lizhimins lizhimins left a comment

Choose a reason for hiding this comment

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

在外面没什么问题。参考:https://juejin.cn/post/7269962653795270671

@joeCarf
Copy link
Contributor

joeCarf commented Dec 1, 2023

在外面没什么问题。参考:https://juejin.cn/post/7269962653795270671

原写法是写到里面了,这个pr是改到外面的

@shenjianeng
Copy link
Author

shenjianeng commented Dec 1, 2023

在外面没什么问题。参考:https://juejin.cn/post/7269962653795270671

这个回复好像容易误导人。

看下 jdk 的官方文档建议吧,应该来说是最“正宗的”,https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/ReentrantLock.html

It is recommended practice to always immediately follow a call to lock with a try block, most typically in a before/after construction such as:

 class X {
   private final ReentrantLock lock = new ReentrantLock();
   // ...

   public void m() {
     lock.lock();  // block until condition holds
     try {
       // ... method body
     } finally {
       lock.unlock()
     }
   }
 }

另外,上面掘金文章结尾有几句话:

  1. 最好是把 lock.lock() 加锁方法写到 try 外面,是一种规范,而不是强制。

  2. 如果你非要写到 try 里面,那么写到 try 语句块的第一行,或者 lock 加锁方法前面不会存在可能出现异常的代码。

  3. 最后,如果你代码中加锁放到了 try 语句里,麻烦参考第 1 点

严格来讲这几个结论,可能在 ReentrantLock 这个实现下是没问题的,但 lock 是接口,会有任意实现多种实现。

在不同的实现下,lock.lock() 方法抛出异常是很有可能的,因此不能放在 try 当中。

@shenjianeng shenjianeng requested a review from lizhimins December 1, 2023 09:36
@fuyou001
Copy link
Contributor

更标准做法,是要放到try外面。

fuyou001
fuyou001 previously approved these changes Dec 18, 2023
Copy link
Contributor

@fuyou001 fuyou001 left a comment

Choose a reason for hiding this comment

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

LGTM

lizhimins
lizhimins previously approved these changes Dec 18, 2023
@shenjianeng shenjianeng dismissed stale reviews from lizhimins and fuyou001 via 17259ae January 29, 2024 01:41
@shenjianeng
Copy link
Author

resolved conflicts

@shenjianeng shenjianeng requested a review from lizhimins January 29, 2024 01:45
@codecov-commenter
Copy link

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (e104273) 43.25% compared to head (17259ae) 43.26%.
Report is 2 commits behind head on develop.

Files Patch % Lines
...he/rocketmq/client/impl/consumer/ProcessQueue.java 61.11% 5 Missing and 2 partials ⚠️
...tmq/namesrv/processor/DefaultRequestProcessor.java 77.77% 1 Missing and 1 partial ⚠️
...a/org/apache/rocketmq/client/impl/MQAdminImpl.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #7582      +/-   ##
=============================================
+ Coverage      43.25%   43.26%   +0.01%     
- Complexity      9880     9884       +4     
=============================================
  Files           1173     1173              
  Lines          85047    85049       +2     
  Branches       11014    11014              
=============================================
+ Hits           36788    36799      +11     
+ Misses         43692    43685       -7     
+ Partials        4567     4565       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

5 participants