[BUG] Builder.Default value sometimes is null

[burdoto]

Describe the bug
Consider this test: https://github.com/codebottle-io/codebottle-java/blob/master/src/test/java/io/codebottle/api/test/RequestTest.java

Every time, the second test method fails for #httpClient field in CodeBottle being null.
This situation could never occur.

To Reproduce
Consider this project: https://github.com/codebottle-io/codebottle-java

Version info (please complete the following information):

• Lombok v1.18.10
• IntelliJ + Gradle

Additional context
Debugger Screenshots:


This is the test setup running before the failing test:

[ghunteranderson]

I believe you're hitting a racing condition bug @burdoto; the worst kind of bug. The constructor generated for CodeBottle will first set instance variable lazyLoading and then set token and httpClient. That's pretty consistent with vanilla java when you mix setting the field at the declaration and setting fields in the constructor. But, in your case, setting lazyLoading means starting two new threads that will be accessing field httpClient. If the threads access httpClient before the constructor can finished running, you will get a null pointer.

I ran a sample test with a custom constructor that waits to create the lazyLoading threads until after httpClient is set and the NullPointerException goes away. I would offer that this is not a bug in Lombok.

    private final @Nullable String token;
    private @Getter final OkHttpClient httpClient;
    public final CompletableFuture<Void> lazyLoading;
    
    public CodeBottle() {
        token = null;
        httpClient = new OkHttpClient.Builder().build();
    	lazyLoading = CompletableFuture.allOf(requestLanguages(), requestCategories());
    }

[burdoto]

Maybe we can get an option to sort the generated parameters and fields?

[ghunteranderson]

Maybe. I'd recommend closing this issue if we feel comfortable that @Builder.Default works as designed and then open a new issue using the feature template for your feature proposal.

[Maaartinus]

Maybe we can get an option to sort the generated parameters and fields?

IMHO better not. There's already one order (the source code order) and you can change it at will. Having one more order would be too confusing.

I'd strongly recommend not to leak this from constructors (or initializers) and not start threads in them. Both is too error-prone. Not sure, whether it's applicable here, but I always prefer preparing everything in the constructor and staring it explicitly later.

---

What's confusing about your code is that the following is perfectly safe

// NO  @Builder.Default
private @Getter final OkHttpClient httpClient = new OkHttpClient.Builder().build();
public final CompletableFuture<Void> lazyLoading = CompletableFuture.allOf(requestLanguages(), requestCategories());

as Java guarantees in-order initialization.

However, with @Builder.Default, the initializer expression must be removed (moved elsewhere) as otherwise the constructor couldn't set the final field httpClient. Unfortunately, the constructor runs after the initializer expressions, so lazyLoading gets set before httpClient.

There's nothing Lombok could do about this apart from moving the other initialization expression into the constructor as well. I'm afraid, in this case it'd be pretty confusing.