CAMEL-20590: avoiding delay to execute timeout to Camel RabbitMQ (InOut)

[rhuan080]

Description

As described on the issue, the https://github.com/apache/camel/blob/camel-3.21.x/components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/reply/ReplyManagerSupport.java#L217 is executing the get excessively. This delay occurs because the ReplyHandler has already been removed by the RabbitMQReplyManagerTimeoutChecker. The contains avoid getting locked when the TimeoutMapEntry does not exist.

Target

• I checked that the commit is targeting the correct branch (note that Camel 3 uses camel-3.x, whereas Camel 4 uses the main branch)

Tracking

• If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

• I checked that each commit in the pull request has a meaningful subject line and body.

• I have run mvn clean install -DskipTests locally and I have committed all auto-generated changes

[github-actions]

🌟 Thank you for your contribution to the Apache Camel project! 🌟

⚠️ Please note that the changes on this PR may be tested automatically.

If necessary Apache Camel Committers may access logs and test results in the job summaries!

[github-actions]

🚫 There are (likely) no components to be tested in this PR

[github-actions]

Core test results:

Tested	Failed ❌	Passed ✅
1	1	0

[github-actions]

🚫 There are (likely) no components to be tested in this PR

[github-actions]

Core test results:

Tested	Failed ❌	Passed ✅
1	1	0

[github-actions]

🚫 There are (likely) no components to be tested in this PR

[github-actions]

Core test results:

Tested	Failed ❌	Passed ✅
1	1	0

[github-actions]

🚫 There are (likely) no components to be tested in this PR

[github-actions]

Core test results:

Tested	Failed ❌	Passed ✅
1	0	1

[davsclaus]

is the map empty, as checking for empty is faster. If you do a exists and then after wards get, then you do x2 the operation.

[rhuan080]

Hi @davsclaus
Yes, it does the operation x2 inside the map, but not inside the DefaultTimeoutMap. Look at this code:

public V get(K key) {
        TimeoutMapEntry<K, V> entry;
        // if no contains, the lock is not necessary
        if (!map.containsKey(key)) {
            return null;
        }
        lock.lock();
        try {
            entry = map.get(key);
            if (entry == null) {
                return null;
            }
            updateExpireTime(entry);
        } finally {
            lock.unlock();
        }
        return entry.getValue();
    }

Without the contains check the lock is applied even if the entry does not exist. The ReplyManagerSupport calls the get method excessively causing a delay.

    ForegroundTask task = Tasks.foregroundTask().withBudget(Budgets.iterationBudget()
                .withMaxIterations(50)
                .withInterval(Duration.ofMillis(100))
                .build())
                .build();

        return task.run(() -> correlation.get(correlationID), answer -> answer != null).orElse(null);

https://github.com/apache/camel/blob/camel-3.21.x/components/camel-rabbitmq/src/main/java/org/apache/camel/component/rabbitmq/reply/ReplyManagerSupport.java#L217

ForegroundTask no longer being able to locate it (this will happen 50 times). Implementing the 'contains' method ensures that the lock is acquired only when necessary.

[davsclaus]

camel-rabbitmq is deprecated and removed in v4. You should use camel-spring-rabbitmq, and can you check what it does in this spring component

[rhuan080]

Sure! I'll test it.
Regarding the update, I believe it's feasible to incorporate it since camel-support is utilized by various components, and it improves the DefaultTimeoutMap avoiding needed lock.

[rhuan080]

I'll come back soon with results of camel-spring-rabbitmq

[davsclaus]

Yeah sure the contains check is outside the lock, but is the locking really a bootleneck. And that frequencey is only every 100ms.

@orpiske ^^

[orpiske]

IMHO, as long as we retain the second if (entry == null) { we should be fine (i.e.: to avoid a TOCTOU issue).

Obs.: without measuring is hard to attest if the lock is a bottleneck , but I think the proposed fix makes sense.