Report a `ReentrantShortLock` that holds a lock for a long time.

[ikhoon]

ReentrantShortLock is designed to allow a locking operation in non-blocking threads. ReentrantShortLock is used cautiously when an operation is finished quickly. It is excluded from the BlockHound rule to avoid false positive reports. https://github.com/line/armeria/blob/d85365147af33b30f76566e13d86144d8e788179/core/src/main/java/com/linecorp/armeria/common/CoreBlockHoundIntegration.java#L36-L37

This causes another issue where a long lock acquisition in ReentrantShortLock is not detected. To solve this problem, it would be nice if there was a way to notify through a warning or metric if ReentrantShortLock is occupying the lock for more than a certain amount of time. (Suggested by @jrhee17)

[JihwanByun]

Would it be okay if I take on this issue?

[ikhoon]

Sure. 👍

[JihwanByun]

@ikhoon I implemented a time-tracking mechanism in ReentrantShortLock to detect long lock acquisitions. The implementation stores the lock acquisition time using a ThreadLocal<Long> and logs a warning if the lock is held longer than the threshold. However, I’m not sure if this approach is efficient, as it tracks the duration for every lock operation.

Since allowBlockingCallsInside is meant to permit blocking calls, I wonder if tracking the duration of all locks is the right approach, or if I might be misunderstanding the issue. Would it be more appropriate to track long-held locks only in specific cases rather than for every lock acquisition? I'd appreciate any feedback or clarification.

[trustin]

@JihwanByun My understanding is that acquiring a lock is already as expensive as System.nanoTime(). Therefore, we can stick to the initial idea:

1. Record the current time on successful lock acquisition at the end of lock()
2. Calculate the lock duration at the beginning of unlock() before unlocking.
3. Log a warning if the duration exceeds a certain threshold, which is configurable via FlagsProvider.

[JihwanByun]

Thank you so much for the feedback, @trustin. I’ll apply your suggestion. Really appreciate your guidance!

[trustin]

At second thought, I feel like we should take the lock/unlock time into account, because it could be useful to find the potential contention that would make it effectively 'blocking'. But still, the overall implementation shouldn't deviate from the initial idea anyway.

[JihwanByun]

Thank you for feedback.

I'm currently using FlagsProvider to make the threshold configurable via getUserValue(...), so users can override it with a JVM option if needed. But I was wondering:

• What would be a reasonable default value for the lock duration threshold? (e.g. 50ms?)
• Do you think it's better to expose it as a user-configurable flag (via getUserValue)? Or would it make more sense to keep it as a fixed default using getValue(...)?

I'd like to hear your thoughts on which approach aligns better with Armeria’s configuration philosophy.