@zhaojiandong For clarity's sake, could you please change the type of the field to ConcurrentMap, & a comment about why it's both @GuardedBy and Concurrent?

// ConcurrentMap even though it is usually accessed synchronized, because it's unsynchronized
// in one case: the onClose function passed into DruidStatements contained by the map. This is
// done to avoid deadlocks; see https://github.com/apache/incubator-druid/issues/6867.
@GuardedBy("statements")
private final ConcurrentMap<Integer, DruidStatement> statements;

ok, I'll put some comments on statements and onClose, thank for review

@zhaojiandong For clarity's sake, could you please change the type of the field to ConcurrentMap, & a comment about why it's both @GuardedBy and Concurrent?

// ConcurrentMap even though it is usually accessed synchronized, because it's unsynchronized
// in one case: the onClose function passed into DruidStatements contained by the map. This is
// done to avoid deadlocks; see https://github.com/apache/incubator-druid/issues/6867.
@GuardedBy("statements")
private final ConcurrentMap<Integer, DruidStatement> statements;

How about use another sync flag: connectionLock, and the type of statements is ConcurrentHashMap ?
In this way GuardedBy check will be passed. @gianm

Why do we need to add @GuardedBy annotation to connectionLock itself here?

Hi @asdf2014 , the field connectionLock now needs to be accessed synchronized, So I think should add an annotation here

Hmmm... Please help me figure out that our initial goal should be to hope that statements can be accessed synchronously. Why not add the @GuardedBy("connectionLock") annotation to the statements property, but add the connectionLock object itself. So I don't understand what this annotation means here.

Because we use field connectionLock as a synchronized flag, connectionLock now protect all methods. we do not need connectionLock to access statements, since the type of statements is ConcurrentMap.

To me, it makes sense because statements is now not always guarded by connectionLock. But, it is in some (well, many) cases. The comment explains it in a way that the annotation would struggle to.

Thanks for explaining this @gianm @zhaojiandong . I mean we shouldn't remove the @GuardedBy("connectionLock") from statements because the statements is not always synchronized by connectionLock and the CI fails, which is unreasonable. In addition, putting a @GuardedBy on connectionLock is also not necessary. If this is done, it means that we should have the connectionLock lock object when we use connectionLock, in fact it cannot achieve the purpose that we should own the connectionLock when using statements. So, i think there is a better way to solve this situation by adding a @SuppressWarnings("FieldAccessNotGuarded") annotation to the Runnable#run method, or adding a // noinspection FieldAccessNotGuarded comment to the statements.remove(statementId) caluse, which will suppress this check and make CI happy.

@GuardedBy("connectionLock")
private final ConcurrentMap<Integer, DruidStatement> statements;
private final Object connectionLock = new Object();

public DruidStatement createStatement(SqlLifecycleFactory sqlLifecycleFactory)
{
    // ...
    final DruidStatement statement = new DruidStatement(
        // ...
        () -> {
          // noinspection FieldAccessNotGuarded
          statements.remove(statementId);
        }
    );
    // ...
}

I raised up a PR #6903, PTAL. @gianm @zhaojiandong

Ah, I see what you mean now @asdf2014 -- your patch #6903 looks good, thanks!

@gianm You are welcome, I am sorry for my poor English expression 😰