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

Empty OneToMany when overflowing LazyLoadBatchSize and mixed queries #3381

Closed
AntoineDuComptoirDesPharmacies opened this issue Apr 2, 2024 · 11 comments · Fixed by #3382
Closed
Assignees
Labels
Milestone

Comments

@AntoineDuComptoirDesPharmacies
Copy link

AntoineDuComptoirDesPharmacies commented Apr 2, 2024

Context

We never had this problem in 4 years. It start doing this since we upgrade from Ebean 13.22.0 to 13.26.1.
We did this upgrade in order to fix the following issue : #3319

We tried to upgrade to 15.0.2 but the issue still exists.

Expected behavior

The linked entites should be loaded from the database.

Actual behavior

We randomly get an empty array when our process access a oneToMany lazy-loaded attribute.
However, the associated entities are well stored in the database and should be returned.

Note : We did not succeed ot reproduce on local development, but this happens once every two days on our production server.

Steps to reproduce

Order entity :

@Entity
public class Order [...] {
    [...]
    @OneToMany(cascade = CascadeType.ALL, mappedBy = "order", fetch = FetchType.LAZY)
    @Column(nullable = true)
    private List<OrderLine> orderLines;
    [...]
} 

OrderLine entity :

public class OrderLine [...] {
  [...]
    @ManyToOne
    @Column(nullable = true)
    private Order order;
  [...]
}

Business code that produce the problem :

        try (Transaction transaction = DB.beginTransaction()) {
            elementsToProcess = Order.find
                    .query()
                    .forUpdate()
                    .where()
                    .isNotNull("dirty")
                    .orderById(true)
                    .setMaxRows(15)
                    .findList();

            process(elementsToProcess.stream().toArray(Order[]::new));

            transaction.commit();
        }
    public void process(Order... orders){
        try (Transaction txn = DB.beginTransaction()) {
            for (Order order: orders) {
                UpdateQuery query = DB.update(Order.class);
                
                [...]
                

	        BigDecimal totalPriceHt = order.getOrderLines() // HERE, THE LINES ARE EMPTY
                                                          .stream()
                                                          .map(PaymentHelper::getTotalPriceHT)
                                                          .reduce(BigDecimal.ZERO, BigDecimal::add);

                query.set("dirty", null)
                        .set("totalPriceHt", totalPriceHt);

                query.where()
                    .eq("id", order.getId())
                    .eq("dirty", order.getDirty())
                    .update(txn);
            }

            txn.commit();
        }
    }
No specific console, everything is running fine, no error, no warn.

Thanks in advance for your help.
Yours faithfully,
LCDP

@AntoineDuComptoirDesPharmacies
Copy link
Author

AntoineDuComptoirDesPharmacies commented Apr 2, 2024

We discovered something !
We successfuly reproduced the problem. It occurs for all Order that are processed above the 10th Order of the array.

We noticed that Lazy Loading in Ebean have a default value of '10'. May it be correlated ?
Ebean Lazy Loading

We tried to call .setLazyLoadBatchSize(3) in the initial Order query and now only the '3' first order processed return the good number of OrderLine. After this, it return empty array.

            elementsToProcess = Order.find
                    .query()
                    .setLazyLoadBatchSize(3)
                    .forUpdate()
                    .where()
                    .isNotNull("dirty")
                    .orderById(true)
                    .setMaxRows(nbProcessed)
                    .findList();

The problem seems to be linked to MixedQuery because we can workaround the problem by computing all UpdateQuery first and apply them at last.

    public void process(Order... orders){
        List<UpdateQuery> updateQueries = new ArrayList<>();

        try (Transaction txn = DB.beginTransaction()) {
            for (Order order: orders) {
                UpdateQuery query = DB.update(Order.class);
                
                [...]
                

	        BigDecimal totalPriceHt = order.getOrderLines() // HERE, THE LINES ARE EMPTY
                                                          .stream()
                                                          .map(PaymentHelper::getTotalPriceHT)
                                                          .reduce(BigDecimal.ZERO, BigDecimal::add);

                query.set("dirty", null)
                        .set("totalPriceHt", totalPriceHt);

                query.where()
                    .eq("id", order.getId())
                    .eq("dirty", order.getDirty());

               updateQueries.add(query);

            }

            for (UpdateQuery updateQuery : updateQueries) {
                txn.setBatchMode(true);
                updateQuery.update();
            }

            txn.commit();
        }
    }

Yours faithfully,
LCDP

@rob-bygrave
Copy link
Contributor

rob-bygrave commented Apr 2, 2024

seems to be linked to MixedQuery

What is MixedQuery? edit: Do you mean transaction.setFlushOnMixed(false) ?

DB.beginTransaction()

Why is the nested beginTransaction() in here? We absolutely need the transaction to wrap the database locks (forUpdate) so the nested beginTransaction() should only be here if this method has multiple entry points and other entry points don't have a transaction [but in creating a failing test case we will not care about other entry points]?

@rob-bygrave
Copy link
Contributor

You are producing a failing test case right?

@AntoineDuComptoirDesPharmacies
Copy link
Author

seems to be linked to MixedQuery

What is MixedQuery?

Maybe we are wrong about the term used, it was a reference to the setFlushOnMixedQuery method but it may not be linked to the flush part because setting True or False on this method do not change anything.

The problem seems to occur when we execute the update at the end of each iteration, and so we have a mixing of Update query and Select (lazy loading) query.
When we store all the UpdateQuery in an array and execute them all together at the end, we do not have the problem of empty array as a result of lazy loading.

Why is the nested beginTransaction() in here? We absolutely need the transaction to wrap the database locks (forUpdate) so the nested beginTransaction() should only be here is this method has multiple entry points and other entry points don't have a transaction?

This lock is only a helper on our side to avoir having multiple process executing the method concurrently but the problem still occur even if we remove it.
The nested beginTransaction() is certainly not useful here. Only to be sure that a transaction is started during the whole execution of this method. We could have use TxScope.mandatory(). We tried to remove this nested transaction and the behavior is unchanged.

You are producing a failing test case right?
Yes, we can reproduce 100% time, do you have a mini project (like a Codesanbox) where I can try to insert the failing code ?

@rob-bygrave
Copy link
Contributor

@rob-bygrave
Copy link
Contributor

query. forUpdate()

Just a note in case I forget to mention it later, that the forUpdate() very likely should be forUpdateSkipLocked() - at least for Postgres, Oracle, Hana, and SQL Server.

@AntoineDuComptoirDesPharmacies
Copy link
Author

query. forUpdate()

Just a note in case I forget to mention it later, that the forUpdate() very likely should be forUpdateSkipLocked() - at least for Postgres, Oracle, Hana, and SQL Server.

What is the concern with forUpdate() compared to forUpdateSkipLocked ? Is there something special aside from the potential infinite lock ?

Here is the reproducer :
Branch where the Asserts are correct, this branch use Ebean 13.22.0

Branch where the Asserts fail on fourth element, considering lazyLoading is set to '3'

@AntoineDuComptoirDesPharmacies AntoineDuComptoirDesPharmacies changed the title Random empty OneToMany when lazy loading Empty OneToMany when overflowing LazyLoadBatchSize and mixed queries Apr 2, 2024
@rbygrave
Copy link
Member

rbygrave commented Apr 3, 2024

upgrade from Ebean 13.22.0 to 13.26.1

Both of these versions do not have this issue/bug.

We did this upgrade in order to fix the following issue : #3320

This doesn't make sense to me in that for #3320 there was NO CHANGE made to ebean at all. That was more that the application needed to change wrt use of transactions in BeanAdapter. So nothing in 3320 requires an upgrade per se.

We tried to upgrade to 15.0.2 but the issue still exists.

Now this issue we are hitting here was actually introduced in versions 14.0.0 / 15.0.0 by #3295 in version . This issue didn't exist prior to that.

@rbygrave
Copy link
Member

rbygrave commented Apr 3, 2024

Bug explanation

The current summary of this bug is that #3295 introduced a behaviour where bulk updates clear the persistence context. Now the lazy loading of a BeanCollection works with an assumption that the "parent bean" is in the persistence context (and this assumption is broken with that change for this test case). That is, the bulk update is clearing the persistence context - removing the parent bean BEFORE the lazy loading is invoked ... and that doesn't then work because the parent bean isn't in the persistence context.

Now for this case we might see that ... if the code just updated the parent (order) beans that would have worked, or if the code had used a stateless update (new order bean populated and then explicitly updated) that would also have worked. This is because for those updates ebean knows it's only updating the single bean.

It is the use of the bulk update + 3295 behaviour to clear the PC + lazy loading collection ... that gives us this bug.

First thought ...

My first thought is to look at the lazy loading collection assuming the parent is in the persistence context.

FYI Stateless update

The stateless update for this case would be:

        // create a new order bean for a stateless update
        var statelessUpdateOrder = new LOrder();
        statelessUpdateOrder.setDirty(null);
        statelessUpdateOrder.setTotal(totalComputed);
        statelessUpdateOrder.setId(lorder.getId());
        // use version for optimistic locking (not using prior dirty value)
        statelessUpdateOrder.setVersion(lorder.getVersion());
        // perform a stateless update
        DB.update(statelessUpdateOrder);
update lorder set dirty=?, total=?, version=? where id=? and version=?; -- bind(null,30.000,2,1,1)

@AntoineDuComptoirDesPharmacies
Copy link
Author

AntoineDuComptoirDesPharmacies commented Apr 3, 2024

As for the issue 3320
You are right, I misslinked, we were talking about issue #3319

As for the versions
You are right about the versioning and bug introduction. At first we switch two versions in our code :

  • Ebean version from 13.22.0 to 13.26.1
  • Play-Ebean to version 8.2.0 (transitive pull Ebean 15.0.1)

Thinking we were running 13.26.1, we were in fact running on Ebean 15.0.1 due to Play-Ebean dependency.

As for your tips about stateless update way of writing.

We do not use this here because we are updating a Bean according to the couple "Id value / Dirty value" and we do not care about version that can change (no optimistic lock check).

Our generated ORM query is :
update lorder set dirty=?, total=? where id=? and dirty=?; -- bind(null,30.000,1,fafddba0-91eb-4ac3-924e-49fd77365130)

rbygrave added a commit that referenced this issue Apr 4, 2024
…d mixed queries

14.0.0 via #3295 introduced this bug.

 #3295 introduced a behaviour where bulk updates clear the persistence context. Now the lazy loading of a BeanCollection works with an assumption that the "parent bean" is in the persistence context (and this assumption is broken with that change for this test case). That is, the bulk update is clearing the persistence context - removing the parent bean BEFORE the lazy loading is invoked ... and that doesn't then work because the parent bean isn't in the persistence context.

 This fix means that when lazy loading many's, the parent beans are putIfAbsent into the persistence context.
rbygrave added a commit that referenced this issue Apr 4, 2024
…d mixed queries

14.0.0 via #3295 introduced this bug.

 #3295 introduced a behaviour where bulk updates clear the persistence context. Now the lazy loading of a BeanCollection works with an assumption that the "parent bean" is in the persistence context (and this assumption is broken with that change for this test case). That is, the bulk update is clearing the persistence context - removing the parent bean BEFORE the lazy loading is invoked ... and that doesn't then work because the parent bean isn't in the persistence context.

 This fix means that when lazy loading many's, the parent beans are putIfAbsent into the persistence context.
rbygrave added a commit that referenced this issue Apr 4, 2024
…or IntelliJ plugin to be released

Adding this means that it will work with an old ebean-agent. It takes a few days to get a new IntelliJ plugin released and we can't wait due to wanting to release the bug fix for #3381
rbygrave added a commit that referenced this issue Apr 4, 2024
Fix for #3381 - Empty OneToMany when overflowing LazyLoadBatchSize an…
@rbygrave rbygrave self-assigned this Apr 4, 2024
@rbygrave rbygrave added the bug label Apr 4, 2024
@rbygrave rbygrave added this to the 14.1.0 milestone Apr 4, 2024
@rbygrave
Copy link
Member

rbygrave commented Apr 4, 2024

FYI: The fix for this is available in versions 14.0.2.1, 14.0.2.1-javax as patch to 14.0.2 and in versions 14.1.0, 14.1.0-javax, 15.1.0 (which include other changes that has already been merged into master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants