projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.93k stars 2.4k forks source link

Be aware of the Cyclic Dependency Issue when using Lombok #806

Closed lombokissues closed 9 years ago

lombokissues commented 9 years ago

Migrated from Google Code (issue 771)

lombokissues commented 9 years ago

:bust_in_silhouette: mchisty   :clock8: Jan 21, 2015 at 19:56 UTC

Hi Guys,

I came up with an issue regarding the cyclic dependency and java.lang.StackOverflowError because of lombok. Here is an explanation of the problem and how it was resolved.

Risks of cyclic relationships between classes.

I had two classes:

@ Entity @ Table(name = "INTERESTED_PARTIES") @ Data public class InterestedParty extends VersionBaseModel {

@ Id @ Column(name = "INT_PARTY_ID") @ GeneratedValue(generator = "NAME_ID_SEQ_GEN") @ SequenceGenerator(name = "NAME_ID_SEQ_GEN", sequenceName = "NAME_ID_SEQ", allocationSize = 1) private Long id;

@ OneToOne(mappedBy = "party", fetch = FetchType.LAZY) private InterestedPartyAddress address; ....

... }

And,

@ Entity @ Table(name = "INTERESTED_PARTY_ADDRESSES") @ Data public class InterestedPartyAddress extends VersionModel {

@ Id @ Column(name = "ADDR_ID") @ GeneratedValue(generator = "ADDR_ID_SEQ_GEN") @ SequenceGenerator(name = "ADDR_ID_SEQ_GEN", sequenceName = "ADDR_ID_SEQ", allocationSize = 1) private Long id;

@ OneToOne @ JoinColumn(name = "INT_PARTY_ID") private InterestedParty party; .... ...

}

Apparently, there is nothing wrong with these classes. However when using lombok's @ Data annotation, the above classes will include additional methods (vanilla java code). IE. <<@ Data generates all the boilerplate that is normally associated with simple POJOs (Plain Old Java Objects) and beans: getters for all fields, setters for all non-final fields, and appropriate toString, equals and hashCode implementations that involve the fields of the class, and a constructor that initializes all final fields, as well as all non-final fields with no initializer that have been marked with @ NonNull, in order to ensure the field is never null.>>

[Source URL: https://projectlombok.org/features/Data.html]

Let's see how the toString() methods will be generated:

For InterestedParty.java, it will produce a toString() method like this:

@ Override public String toString() { return "InterestedParty [id=" + this.getId() + ", address=" + this.getAddress() + "]"; }

For InterestedPartyAddress.java, it will produce a toString() method like this:

@ Override public String toString() { return "InterestedPartyAddress [id=" + this.getId() + ", party=" + this.getParty() + "]"; }

And here is the problem. As seen from the toString() methods, the InterestedParty class and the InterestedPartyAddress class now have a cyclic relationship created because of the toString() methods. A InterestedParty instance has a reference to its InterestedPartyAddress in the toString() method and a InterestedPartyAddress has a reference to the InterestedParty's toString() method. As a result, a StackOverflowError occurs when the respective toString() methods of each object try to call one another.

Error: Caused by: java.lang.StackOverflowError at java.lang.StringBuilder.<init>(StringBuilder.java:85) [rt.jar:1.7.0_60] at au.gov.nsw.osr.mars.model.duties.InterestedParty.toString(InterestedParty.java:33) [mars-ejb.jar:] at java.lang.String.valueOf(String.java:2847) [rt.jar:1.7.0_60] at java.lang.StringBuilder.append(StringBuilder.java:128) [rt.jar:1.7.0_60]

Error detail is attached with this email.

Solution: Generally this is not an issue if it is a one directional relationship. But in case of a bi-directional relationship, it can cause clyclic dependency hence causing big problem. We can solve it either of the following two ways:

  1. If using lombok's @ Data annotation, then resolve by using the @ ToString(callSuper=true, includeFieldNames=false) on a specific field that we do not want to be included in the toString() Pro: Less code. Cons: Debugging pain, need to be cautious about cyclic dependency. Failing to do such might cause StackOverflow error. Also sometimes we do not want getter-setter for certain fields. In such a case, we must not forget to use @ Getter(AccessLevel.NONE) or @ Setter(AccessLevel.NONE) in a field level
  2. By NOT using @ Data. Instead, using Eclipse's getter-setter, toString(), hasCode() generate feature Pros: Debugging made easy. No need to worry about cyclic dependency, we can decide which attributes/properties should be included while generating getter-setter/toString/hasCode etc Con: Verbosity

In my case, I resolved the issue by following step 2 i.e. get rid of the lombok's @ Data annotation and producing getter-setters with Eclipse. I did not need toString() at all.

A further discussion on clyclic relation can be found here: http://marxsoftware.blogspot.com.au/2009/07/diagnosing-and-resolving.html

Thanks. ... Chisty

lombokissues commented 9 years ago

:bust_in_silhouette: Maaartinus   :clock8: Jan 24, 2015 at 23:40 UTC

I resolved the issue by ... get rid of the lombok's @ Data annotation and producing getter-setters with Eclipse.

IMHO that's an outstandingly terrible solution. You could add a trivial toString calling super and be done. You could also use ToString(exclude="InterestedParty") in order to break the cycle. You could replace @ Data by @ Getter, @ Setter, and whatever else you need.

While Lombok could handle cycles, it's not free. The cost would be much longer and slower code, auxiliary methods, thread-local magic, and/or a runtime dependency. Note that neither JDK containers, nor Guava's and Apache's ToStringBuilder, nor any class I've ever seen protects against cycles.

lombokissues commented 9 years ago

:bust_in_silhouette: mchisty   :clock8: Jan 25, 2015 at 06:56 UTC

As you said <<IMHO that's an outstandingly terrible solution. You could add a trivial toString calling super and be done. You could also use ToString(exclude="InterestedParty") in order to break the cycle. You could replace @ Data by @ Getter, @ Setter, and whatever else you need.>>

If you look at my comments carefully, I already mentioned it in point ﹟1.

Terrible solution !!! Well, you could say that because as a Lombok tool developer, you will want to promote your tool. I can't blame you.

Also... what is your explanation about debugging? Lombok is just good for removing verbosity (or less code); that's all.

Thanks.

lombokissues commented 9 years ago

:bust_in_silhouette: r.spilker   :clock8: Jan 27, 2015 at 16:54 UTC

For your information: the java standard libraries are also prone to stack overflow problems. Try out the following code:

List list = new ArrayList<>(); list.add(list); String content = "" + list;

As one of the core lombok developers, I am biased. I think @ ToString(exclude={"party"}) is very clear and concise.

lombokissues commented 9 years ago

:bust_in_silhouette: mchisty   :clock8: Jan 27, 2015 at 20:23 UTC

I agree. Java standard libraries are also prone to StackOverflow. However in the above scenario, the cyclic dependency was my main problem which caused the build broken. The StackOverflow error occurred in this case because of the cyclic dependency. Like I mentioned in my original post, developers won't even have to worry about using ToString at all unless and until there is a bi-directional relationship. In any other cases, no issues.

Anyway, you did not answer anything about the debugging problem (debugging of getter-setters). Any clue or suggestion?

Thanks.

lombokissues commented 9 years ago

:bust_in_silhouette: Maaartinus   :clock8: Jan 28, 2015 at 18:12 UTC

﹟2 FYI, I'm not a Lombok developer (not even a contributor apart from some tiny things), just a fan.

If you look at my comments carefully, I already mentioned it in point ﹟1.

You did, but you chose the worse solution. YMMV. For me, keeping the source compact and clean is very important. And yes, that's all Lombok can do.

Concerning debugging, I guess you got no answer because there's no such question? At least, I can't see it. When I run into a problem with a ToString cycle, I just fix it. I don't try debug into trivial generated methods, just like I don't try debug into bridge methods. It wouldn't work, but I couldn't care less as there's nothing interesting there.

lombokissues commented 9 years ago

:bust_in_silhouette: mchisty   :clock8: Jan 28, 2015 at 22:08 UTC

As you mentioned <<When I run into a problem with a ToString cycle, I just fix it. I don't try debug into trivial generated methods>>.

Answer: Neither do I. Let me explain: say you have a class with lomboks' @ Data annotated. Now read carefully: I an NOT talking about cyclic dependency and ToString annotation. It is a different topic. (Cyclic dependency happens only when there is bi-directional relationship and developer do not use ToString as you mentioned). Now I will explain the debugging issue.

Let's say, at some point, your application is throwing an exception from UI layer. E.G. you tick a checkbox in your page, you expect a boolean property (someProperty) in your class to be true but it seems like the value is is still false. What could be wrong? What is causing the value not to be updated? Is ajax refresh working? Is the page re-rendered after a onclick event..... etc .. etc.. lot of question. So you might want to debug whether the property values are being set/updated correctly; if so, where or at what point; you might want to set the debug point on isSomeProperty(), setSomeProperty() methods [don't say you do not do debugging and you fix code without debugging). Can you do that? You can't. Because there is NO is/get/set methods. Got it?

<<Concerning debugging, I guess you got no answer because there's no such question?>>

Answer: I do not get it. Do you really have the answer? There is no such question just because nobody asked it before? What's your point?

Thanks.

P.S. I am not expecting more answers like "I don't try debugging... I just fix it... there is no such questions " ..... :). If you do not have the answer, admit it.

lombokissues commented 9 years ago

:bust_in_silhouette: r.spilker   :clock8: Jan 29, 2015 at 17:16 UTC

For debugging purposes you can always use delombok to (temporarily) convert your files to plain java and use those files to compile and debug your project.

That said, there should really be no need to step into, say, a getter. Getters and setters are simple. They do what they are supposed to do. If you get an unexpected value, it's not the getter or setter that failed, but the one calling the setter or constructor.

The Eclipse Outline View is also very helpful. If you right click on the lombok generated method there are several useful options.

Toggle Method Breakpoint will break when the method is called. Although you cannot see the actual source of the method, the Variables view will display the values of the parameters. And Step Into, Over or Return will step to the calling code. The Breakpoint View allows you to set more properties, allowing for conditions, for instance to only break when a certain parameter is set, or to break on exit, allowing inspection of state of the object.

The References option allows you to quickly find the callers of the lombok generated code.

All in all, I think there are enough options to debug your lombok laced code.

lombokissues commented 9 years ago

End of migration