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

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

Merged
merged 1 commit into from
Mar 21, 2024

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.

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.

@jerqi
Copy link
Contributor

jerqi commented Mar 19, 2024

@zuston Rust CI still is unstable.

@jerqi
Copy link
Contributor

jerqi 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

@jerqi jerqi 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

@jerqi jerqi merged commit 32d533d into apache:master Mar 21, 2024
41 checks passed
@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.

None yet

2 participants