Skip to content

[MINOR] refactor: Calling lock() method outside try block to avoid unnecessary errors#1590

Merged
roryqi merged 1 commit intoapache:masterfrom
rickyma:minor-refactor-lock-outside-try
Mar 21, 2024
Merged

[MINOR] refactor: Calling lock() method outside try block to avoid unnecessary errors#1590
roryqi merged 1 commit intoapache:masterfrom
rickyma:minor-refactor-lock-outside-try

Conversation

@rickyma
Copy link
Contributor

@rickyma rickyma commented Mar 18, 2024

What changes were proposed in this pull request?

Calling lock() method outside try block to avoid unnecessary errors

Why are the changes needed?

In general, the lock() method should be placed outside the try block. The reason is that if the lock() method throws an exception, it indicates that the lock was not acquired, so there is no need to attempt to release it in the finally block. If the lock() method is placed within the try block and it throws an exception, the finally block will still be executed and attempt to release a lock that was never acquired, leading to errors.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs.

@github-actions
Copy link

Test Results

 2 340 files  ±0   2 340 suites  ±0   4h 32m 8s ⏱️ -48s
   908 tests ±0     907 ✅ ±0   1 💤 ±0  0 ❌ ±0 
10 541 runs  ±0  10 527 ✅ ±0  14 💤 ±0  0 ❌ ±0 

Results for commit dbd6b83. ± Comparison against base commit f3775a1.

@roryqi
Copy link
Contributor

roryqi commented Mar 19, 2024

@zuston Rust CI still is unstable.

@roryqi
Copy link
Contributor

roryqi commented Mar 19, 2024

Is it a best practice for the lock? Any other projects follow the rule?

@rickyma
Copy link
Contributor Author

rickyma commented Mar 21, 2024

Is it a best practice for the lock? Any other projects follow the rule?

The Java documentation, however, leaves lock() outside the try block in the ReentrantLock example. The reason for this is that an unchecked exception in lock() should not lead to unlock() incorrectly being called.

You can refer to Java Doc.

It is described in:
https://stackoverflow.com/a/10868476.

Copy link
Contributor

@roryqi roryqi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @rickyma

@roryqi roryqi merged commit 32d533d into apache:master Mar 21, 2024
@rickyma rickyma deleted the minor-refactor-lock-outside-try branch May 5, 2024 08:33
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