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

NPE in NullPointerException in ExpressionOperator.printCollection since 2.7.11+, 3.0.0+ #2136

Open
igormukhin opened this issue May 9, 2024 · 4 comments · Fixed by #2139

Comments

@igormukhin
Copy link
Contributor

igormukhin commented May 9, 2024

Describe the bug

Since switching to EL 2.7.14 we are getting a mysterious NPE at ExpressionOperator.printCollection().
The exception happens only occasionally from time to time (like once a week) on different queries.

It also happend to other people #1867 and #1717

To Reproduce

I don't know how to reproduce it, but it is clearly a bug in the source code. Read further.

UPDATE 10.06.2024
I created a sample project that reproduces the issue: https://github.com/igormukhin/eclipselink-issue-1717-test
The problem is when a queries are running in parallel threads and use COALESCE(...) function or CASE WHEN statement.

Expected behavior

Same as before v2.7.11.

UPDATE 10.06.2024 The following problem description and the solution can be ignored as it is invalid.

The Problem

In the class ExpressionOperator there is a field argumentIndices

Up to 2.7.10 the field was used in the method printCollection like this:

    public void printCollection(List<Expression> items, ExpressionSQLPrinter printer) {
        // ... skipped some code ...

        if (this.argumentIndices == null) {
            this.argumentIndices = new int[items.size()];
            for (int i = 0; i < this.argumentIndices.length; i++){
                this.argumentIndices[i] = i;
            }
        }

        // ... skipped some code ...
        for (final int index : argumentIndices) {
            Expression item = items.get(index);
            // ... skipped some code ...
                item.printSQL(printer);
           // ... skipped some code ...
        }
    }

In 2.7.11 the line for (final int index : argumentIndices) was changed to for (int i = 0; i < this.argumentIndices.length; i++)
That is causing the exception.

The field argumentIndices can be changed while inside the loop because the loop calls item.printSQL(printer) which in turn can call ExpressionOperator.copyTo where the field argumetIndices on the already used operator instance will be set to null.

I can't explain why it does work most of the time and only occasionally fails but it is clearly the reason for the NPE.

Solution

While it is not the ultimate solution for the underlying problem (the field can be changed), we still can fix it with just by returning back to caching the value of argumentIndices while looping which is already done in 3.1.0-M1 and 4.0.0 but not in 2.7.x, 3.0.x

igormukhin added a commit to igormukhin/eclipselink that referenced this issue May 9, 2024
…ator.printCollection since 2.7.11

The bug is described in detail in eclipse-ee4j#2136

We are looping over an array which is located in the instance field which can be set to `null` while lopping which is causing NPE.
To fix it we are caching the value of `argumentIndices` by looping over an iterator of the array.
rfelcman pushed a commit that referenced this issue May 10, 2024
* fixes #2136 NPE in NullPointerException in ExpressionOperator.printCollection since 2.7.11
The bug is described in detail in #2136
We are looping over an array which is located in the instance field which can be set to `null` while lopping which is causing NPE.
To fix it we are caching the value of `argumentIndices` by looping over an iterator of the array.
rfelcman added a commit that referenced this issue May 14, 2024
Fixes #2136 
Backport from #2137
---------

Signed-off-by: Radek Felcman <radek.felcman@oracle.com>
rfelcman added a commit that referenced this issue May 14, 2024
Fixes #2136 
Backport from #2137

Signed-off-by: Radek Felcman <radek.felcman@oracle.com>
@jnehlmeier
Copy link

The issue doesn't seem to be fixed as noted in latest comment in #1717

2.7.15 has this fix applied applied but is still affected.

@igormukhin
Copy link
Contributor Author

igormukhin commented Jun 10, 2024

I was wrong with my PR #2137. My solution helped nothing.

So I just created a sample project that reproduces the issue:
https://github.com/igormukhin/eclipselink-issue-1717-test

The problem is the concurrency and COALESCE function or CASE WHEN.

@rfelcman Can you please reopen this issue? I can then try to hunt down the error.

@rfelcman rfelcman reopened this Jun 11, 2024
@igormukhin
Copy link
Contributor Author

igormukhin commented Jun 11, 2024

The root of all evil here is probably the fact that DatabasePlatform provides the same instance of ExpressionOperator
which has a state field (argumentIndexes). This, of course, leads to errors when the same operator is used in different threads.

Currently, that involves ArgumentListExpressionOperator (and its parent ListExpressionOperator) which is used in these expressions:

        addOperator(ExpressionOperator.coalesce());
        addOperator(ExpressionOperator.caseStatement());
        addOperator(ExpressionOperator.caseConditionStatement());

It look like somebody tried to solve the issue years ago (Bug 463042) but not so successfully.
We probably should look what happened to these classes in the version 2.7.12 where the problem was introduced.
Will take a look later.

@igormukhin
Copy link
Contributor Author

Looks like it was this commit with changes in ArgumentListFunctionExpression.java that introduced the issues.

ajaypaul-ibm pushed a commit to ajaypaul-ibm/eclipselink that referenced this issue Jun 24, 2024
rfelcman pushed a commit that referenced this issue Sep 5, 2024
… (#2168)

The bug is described in the issue #2136

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ExpressionOperator.java - printCollection method is modified so it does not modify the shared argumentIndexes field.
ArgumentListFunctionExpression.java - previous "workarounds" are removed

Co-authored-by: Igor Mukhin <igor.mukhin@capgemini.com>
Igor-Mukhin added a commit to igormukhin/eclipselink that referenced this issue Sep 11, 2024
…different number of arguments

The bug is described in the issue eclipse-ee4j#2136

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ArgumentListFunctionExpression.java - fixes printSQL to use the operator itself and not the common instance of it
Igor-Mukhin added a commit to igormukhin/eclipselink that referenced this issue Sep 11, 2024
…different number of arguments

The bug is described in the issue eclipse-ee4j#2136

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ArgumentListFunctionExpression.java - fixes printSQL to use the operator itself and not the common instance of it
Igor-Mukhin added a commit to igormukhin/eclipselink that referenced this issue Sep 11, 2024
…different number of arguments (backport to 4.0)

The bug is described in the issue eclipse-ee4j#2136. The bug was in the 2.7 version, and it was somewhat fixed in later versions but still failed when the statements had different number of parameters.

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ArgumentListFunctionExpression.java - fixes printSQL to use the operator itself and not the common instance of it.
Igor-Mukhin added a commit to igormukhin/eclipselink that referenced this issue Sep 11, 2024
…different number of arguments

The bug is described in the issue eclipse-ee4j#2136

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ExpressionOperator.java - fixes getArgumentIndices by disabling the caching of the dynamically created argument indexes.
rfelcman pushed a commit that referenced this issue Sep 12, 2024
…different number of arguments (#2259)

The bug is described in the issue #2136

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ArgumentListFunctionExpression.java - fixes printSQL to use the operator itself and not the common instance of it
ExpressionOperator.java - fixes getArgumentIndices by disabling the caching of the dynamically created argument indexes.

---------

Co-authored-by: Igor Mukhin <igor.mukhin@capgemini.com>
Igor-Mukhin added a commit to igormukhin/eclipselink that referenced this issue Sep 12, 2024
…different number of arguments (backport to 4.0)

The bug is described in the issue eclipse-ee4j#2136.

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ExpressionOperator.java - fixes getArgumentIndices by disabling the caching of the dynamically created argument indexes.
Igor-Mukhin added a commit to igormukhin/eclipselink that referenced this issue Sep 12, 2024
…different number of arguments (backport to 4.0)

The bug is described in the issue eclipse-ee4j#2136.

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ExpressionOperator.java - fixes getArgumentIndices by disabling the caching of the dynamically created argument indexes.
Igor-Mukhin added a commit to igormukhin/eclipselink that referenced this issue Sep 12, 2024
…different number of arguments (backport to 3.0)

The bug is described in the issue eclipse-ee4j#2136.

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ExpressionOperator.java - fixes getArgumentIndices by disabling the caching of the dynamically created argument indexes.
rfelcman pushed a commit that referenced this issue Sep 13, 2024
…different number of arguments (backport to 4.0) (#2262)

The bug is described in the issue #2136.

TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug.
ArgumentListFunctionExpression.java - fixes printSQL to use the operator itself and not the common instance of it
ExpressionOperator.java - fixes getArgumentIndices by disabling the caching of the dynamically created argument indexes.

Co-authored-by: Igor Mukhin <igor.mukhin@capgemini.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants