Lombok: Synchronized with Reentrantlock

[lombokissues]

_Migrated from Google Code (issue 575)_

[lombokissues]

:bust_in_silhouette: theaspect   :clock8: Sep 17, 2013 at 05:18 UTC

Can you add ability to generate from this:
@ Reentrant(lock)
public static hello(){
System.out.println("world");
}

that:
public static void hello() {
lock.lock();
try{
System.out.println("world");
}finally{
lock.unlock
}
}

[lombokissues]

:bust_in_silhouette: askoning   :clock8: Oct 07, 2013 at 20:13 UTC

[lombokissues]

:bust_in_silhouette: Maaartinus   :clock8: Oct 15, 2013 at 14:39 UTC

This could be extended by ReadLock and WriteLock (and whatever...). I guess, the distinction between read and write access is the most common reason why synchronized (or @ Synchronized) can't be used.

Concerning the syntax, maybe

@ Lock(lock)
@ Lock.Read(lock)
@ Lock.Write(lock)

could do. Omitting the argument could use a lombok-generated lock (just like @ Synchronized does).

[lombokissues]

_End of migration_

[rzwitserloot]

We can't use @Lock(lock), but I assume that was meant to read as @Lock("lock")?

[1] in any case let's add a detector that tries to figure out if you're using a common-ish java.util.concurrent.locks lock in @Synchronized and emit a warning, because synchronizing on a lock object is weird.

[2] I'd love for all this to just be part of @Synchronized, but then we're kinda forced into either making you supply an enum as param which is unwieldy, or trying to auto-detect if the var is a lock. Heuristically we can get close, but as Lock is an interface, we can't be sure, so that's right out. We must have a new annotation then.

Once we have the annotation, what, exactly, should be done? The 4 common lock classes are:

• java.util.concurrent.locks.ReadLock
• java.util.concurrent.locks.WriteLock
• java.util.concurrent.locks.ReentrantLock
• java.util.concurrent.locks.ReentrantReadWriteLock

Which do we generate when, and what lock code do we generate? Are there obvious answers?

[Maaartinus]

We can't use @Lock(lock), but I assume that was meant to read as @Lock("lock")?

I guess, I didn't think much about that.

We must have a new annotation then.

I guess so.

public @interface Locked {
    // The default (e.g., $lombok$lock) must not collide with what @Synchronized uses.
    // As with @Synchronized, for static methods another default name gets used
    // (e.g., $lombok$LOCK).
    String name() default "";

    // When either false or true is ever explicitly given for a given name,
    // then a read-write lock gets generated.
    // Otherwise, a simple lock gets generated.
    // Leaving it out for a given name should probably be an error,
    // when it was given elsewhere.
    boolean write() default true;

   // When any annotation for a given name specifies true,
   // then the generated lock is reentrant.
   // When any annotation for a given name specifies false,
   // then the generated lock is non-reentrant.
   // With two conflicting values for a given name, error out.
   // When nothing is specified for a given name,
   // then the generated lock is reentrant.
   // When the lock already exists, this option must be left unset.
   boolean reentrant() default true;
}

[adwsingh]

Does that mean @Locked feature approved? In which case I would like to work on this.

[rzwitserloot]

@mehwater Let's first work on what, precisely, is generated by what. What would happen for:

[1] @Locked
[2] @Locked(name = "hello")
[3] @Locked(reentrant = false)
[4] @Locked(write = true)
[5] @Locked(write = false)
[6] @Locked(write = true, reentrant = false)
[7] @Locked(write = false, reentrant = false)

also, having the write field be a secret trinary value (false, true, or unspecified all having different behaviours) feels weird, maybe we should go with an enum instead? Maybe bring reentrant in on that too and just go with 'type'. Why would you want a non-reentrant one?

I don't program a lot with locks, maybe you can tell :)

[Maaartinus]

@rzwitserloot You're commenting on my proposal, so I'm answering:

having the write field be a secret trinary value (false, true, or unspecified all having different behaviours) feels weird, maybe we should go with an enum instead?

I know, but enums are a pain to write. The idea was that specifying the option implies that you care about write/read behavior and leaving it out means that a simple lock is good enough.

Possibly, using @Locked for simple locks and @Locked.Read and @Locked.Write would be better. NB: a read-write lock is no lock at all, it's a container for two coupled locks. You never create a write (or read) lock yourself; you create a read-write lock and use a part of it (see [4] below).

Combinations like @Locked and @Locked.Read make no sense and must be forbidden.

The nice thing about the enum is that it actually fits with @Synchronized:

public @interface Synchronized {
    public enum Type {
         DEFAULT, // as before
         LOCK, // normal ReentrantLock
         READ,
         WRITE,
    }
    ...
}

For a given name, all type combinations are forbidden except for READ+WRITE.

Why would you want a non-reentrant one?

Efficiency is the only reason I can imagine. I found one implementation in my Eclipse assuming it belongs to JDK (or maybe Guava), but it was netty. So let's forget it, the efficiency gain is obviously considered to be unimportant.

There's another problem with my idea: reentrancy exists per lock rather than per method. So it makes little sense to pack it into the annotation.

---

[1] @Locked

• The same as `@Locked(THE_DEFAULT_LOCK_NAME_WHATEVER_IT_IS).

[2] @Locked(name = "hello")

• Enclose the method body in hello.lock(); try {ORIGINAL_METHOD_BODY} finally {hello.unlock();}.
• When there's no field named "hello", then generate private final Lock hello = new ReentrantLock();.
• Do some sanity checks on the field (it mustn't be used by @Synchronized or as read-write lock).

[3] @Locked(reentrant = false, name = "hello")

• The same as [2], except for generating .... = new NonReentrantLock();, but let's forget it as it's unavailable.

[4] @Locked(write = true, name="hello")

• Enclose the method body in hello.writeLock().lock(); try {ORIGINAL_METHOD_BODY} finally {hello.writeLock().unlock();}
• When there's no field named "hello", then generate private final ReadWriteLock hello = new ReentrantReadWriteLock();.
• Do some sanity checks on the field (it mustn't be used by @Synchronized or as a plain Lock).

[5] @Locked(write = false, name="hello")

• Same as [4], just with readLock in place of writeLock.

[6] @Locked(write = true, reentrant = false, name="hello")

• Combination of [4] and [3], so forget it.

[7] @Locked(write = false, reentrant = false, name="hello")

• Combination of [5] and [3], so forget it.

---

There's another option: A lock can be fair or not. This is to be specified as a constructor argument, so it doesn't fit into the annotation well (just like reentrancy). As a fair lock is much slower than a normal one, it probably gets hardly ever used, so a user should define the field themselves when needed.

[rzwitserloot]

Feature proposal approved; here's the implementation we're looking for (it's basically @Maaartinus 's idea):

Feature specification

@lombok.Lock, @lombok.Lock.Read, and @lombok.Lock.Write are the annotations (the latter 2 are inner annotations, see how @Builder.Default works; we're skipping experimental for this one. They each have one parameter named value of type String, defaulting to empty, which means lock if on an instance method, and LOCK if on a static method. Otherwise, it means precisely the string as provided.

The rules are: The field, if it exists and written by the user, it will be used as is (and if the field is of the wrong type or wrong staticness, just generate the code. The compiler will error and that is fine). If it does not exist, it is generated; @Lock makes a ReentrantLock (field type j.u.c.l.Lock, it is final, initializing expression is new j.u.c.l.ReentrantLock(), and it is static if the annotated method is static.The read/write variants make a ReadWriteLock. If it is already there and generated by lombok, check if there's a type conflict (i.e. You mixed @Lock with @Lock.Read/@Lock.Write and did not manually write the lock field, or you mixed non-static with static), in which case (so, the type of that field is not the one you expected, or the static flag is different from what you would have generated), produce an error explicitly outlining the incompatibility.

The method body is replaced with lockVar.lock(); try { BODY } finally { lockVar.unlock(); }, replacing that with lockVar.readLock().lock() / lockVar.writeLock().lock() for the read/write variants.

Mixing @Synchronized and @Lock is bizarre but there is no need to attempt to detect is, and the actual ordering (is it a synchronized() body, within which the lock is locked, or does the lock lock, then a try/finally block, whose body is then entirely synchronized()?) – Doesn't matter. Anybody who types that is clearly ASKING for crazy so we'll just deliver :)

Design defense

The above deviates rather heavily from how @Synchronized works, which is why the charming solution of the type enum isn't actually what we decided on:

• The autogen name for @Synchronized is $lock, and with that dollar we are explicitly trying to suggest that you shouldn't directly interact with this object. Because there is as far as I know absolutely no point to using a j.u.c.l.Lock over a synchronized() based lock unless you _interact_ with that lock (for example, because you want to explicitly invoke tryLock at some point, a feature that synchronized() does not offer at all, or you want to check if it is locked or not), therefore, a dollar in the name is not the proper way to do this.

• @Synchronized does not generate the lock field unless you leave the name blank; for the same reason (given that you specified the name you likely want to interact with it, but for @Synchronized we didn't want that). We have no problem with that here, so for @Lock the field is generated even if you pick an explicit name.

I can imagine that users would prefer the j.u.c.l variant by default, so if we go with the "@Synchronized now gets a type param" approach, either users will put in the whole unwieldy mess everywhere they use it, or we need to add a lombok.config key. This all gets sufficiently hairy that just introducing another annotation is the better cost/benefit ratio.

Open questions

More a meta-open-question: Does anyone foresee issues here; issues which would make us regret skipping the experimental package entirely? If we skip it, making backwards breaking changes later is painful. The reason we want to skip experimental is because we don't foresee issues with the maintenance burden (this is all very straightforward), and don't foresee having to break compatibility due to a better idea coming along, but if you feel that we're not thinking creatively enough and you DO foresee awesome ways to take this feature that would probably require breaking changes, do pipe up with crazy scenarios!

[Maaartinus]

Because there is as far as I know absolutely no point to using a j.u.c.l.Lock over a synchronized()

Neither I know any, but there's a reason of using a read-write lock without interacting with it manually. Still, using no dollars in the name is fine.

I was thinking about using a different name for the read-write lock. When using both @Lock and @Lock.Read/Write, you need two different fields, but I can't find any good name and this case is probably not worth complicating the clean design.

---

The only thing I dislike is the name Lock, which conflicts with j.u.c.Lock. In the simple case, the field gets generated by Lombok and there'll be no occurrence of j.u.c.Lock in the source, that's fine. But in more complicated cases, the user will be forced to spell out the FQN. That's why I'd prefer Locked - longer, but conflict-free and on par with @Synchronized.

Concerning "experimental", I'd suggest to use it as there was no feedback from anyone but me and you. I use all features I need, experimental or not, and see the experimental package as a chance to start over on a clean ground. The chance of needing it is low, but the cost is even lower.

[adwsingh]

@rzwitserloot I see the specifications seem to be pretty much close upon. I agree with @Maaartinus to put this in experimental and wait for feedback before promoting it.

So can I start on the PR?

[rzwitserloot]

@mehwhatever go for it! – two changes to what I originally wrote:

1. Let's start in experimental after all.
2. Let's call it @Locked, because name-conflicting with j.u.c.l.Lock does sounds quite annoying.