Improve thread safety of RedisMessageListenerContainer.

[jebeaudet]

• [X] You have read the Spring Data contribution guidelines.
• [X] You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• [X] You submit test cases (unit or integration tests) that back your changes.
• [X] You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Hi,

As instructed in #964, a PR to fix the issues I've encountered in RedisMessageListenerContainer.

I've commented the PR inline to ease the comprehension. Let me know if you need further details.

Testing those kind of multithreaded behavior is never easy but I believe all my additions have been tested in the unit tests I've added.

[mp911de]

I'm a bit reluctant to backport these changes, especially towards a version that is GA since over a year. Changes to RedisMessageListenerContainer have shown that they are likely to cause all sorts of synchronization problems due to their highly concurrent nature.

The actual problem with Jedis and RedisMessageListenerContainer is that the startup/lazy listening lets the other processes proceed although we do not have a confirmation response from Redis. That can lead to concurrent usage of the OutputStream.

I strongly argue to leave the revised implementation on the 2.7.x branch, especially since the GA release is scheduled for May 13. I would also strongly recommend upgrading to the 2.7 RC1 release to verify that the revised implementation solves more problems than it creates.

[jebeaudet]

Thanks for you insight.

While I agree on the GA part, to me, it's still a bug and bugs should be fixed. Especially this one since you could have a pattern subscription fail completely silently or a corrupted jedis stream. Also, version 2.5.x is the one managed from Spring Boot 2.5.x which is still in support for another month and EoL in August 2023 if my math is correct. Even Spring Boot 2.6.x, which is the current stable version, does not pull on the 2.7.x branch. This means that everyone using Spring Boot with a released version is currently affected.

I'd also argue that since Spring Session is using the problematic code path (having a topic + pattern subscription), it affects even more users and could potentially affect users that rely on SessionCreatedEvent.

I guess we could shuffle things a bit to reduce the footprint of the changes. Basically this PR fixes :

1. The non thread safe usage of JedisSubscription
2. The non resilient behavior of the PatternSubscriptionTask (exceptions are uncaught and it only retries 3 times)

Just synchronizing JedisSubscription would avoid corrupting the jedis stream and would not add much overhead. With this, we could bump the ROUNDS variable along with a larger try-catch to make sure the PatternSubscriptionTask does not exit silently and keep trying. This would reduce the changes a bit and we could target this PR against the 2.7.x branch and the proposed one agains 2.5.x and 2.6.x.

Unless you have another idea?

Thanks again

[jebeaudet]

WDYT @mp911de ?

Thanks

[mp911de]

Version 2.5 is no longer maintained.