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

Correctly handle null listener in RedisMessageListenerContainer.remove(…) #3009

Conversation

sujl95
Copy link
Contributor

@sujl95 sujl95 commented Sep 29, 2024

Hello,

While using the remove API, I encountered a NullPointerException (NPE) when the listener parameter was null. This PR addresses that issue by updating the remove method in RedisMessageListenerContainer.

The proposed changes ensure that when the listener is null, all listeners associated with the specified topic are removed. Additionally, the listenerTopics and mapping collections are cleaned up properly to prevent NPEs, making the listener removal process more robust.

If this contribution does not meet any of the project’s contribution requirements, please feel free to let me know, and I’ll be happy to make any necessary adjustments.

Summary:

This PR improves the remove() method in RedisMessageListenerContainer by ensuring that it gracefully handles null listeners. When the listener parameter is null, the method will now remove all associated listeners for the specified topic. The method has been updated to prevent NullPointerExceptions (NPEs) and ensure proper cleanup of the listenerTopics and mapping collections.

Description of the problem:
In the current implementation of RedisMessageListenerContainer.remove(), a potential NullPointerException (NPE) can occur when the listener parameter is null. This leads to unexpected behavior when trying to remove listeners for a specific topic, especially when no listeners are associated with the topic, or the listener is null.

Without proper handling, the method can attempt to remove a null listener or update the listenerTopics and mapping in a way that results in an NPE, disrupting the listener removal process.

Proposed solution:
This PR updates the remove() method in RedisMessageListenerContainer to correctly handle cases where the listener parameter is null. Specifically:

  • When listener is null, all listeners associated with the specified topic are removed.
  • The method ensures that listenerTopics and mapping collections are updated correctly by checking if collections are empty before removal.
  • CollectionUtils.isEmpty() is used to ensure proper cleanup of empty collections and avoid NPEs.

These changes ensure that the listener removal process is more robust and handles edge cases involving null listeners and empty mappings more gracefully.

Changes:

  • Updated the remove() method in RedisMessageListenerContainer to:
    • Remove all listeners when the listener parameter is null.
    • Check for empty collections before removing listeners and update listenerTopics and mapping accordingly.
    • Avoid NPEs by utilizing CollectionUtils.isEmpty() for safer collection handling.

How to reproduce the issue:
The issue can be triggered by calling the remove() method with a null listener while there are no listeners associated with the topic in listenerTopics. This would result in an NPE when trying to update the collections.

Testing:

  • Unit tests have been added and modified to ensure that the method works as expected when the listener is null.
  • The tests verify that no NPE occurs and that listeners are correctly removed in both the single listener and multiple listeners scenarios.
  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • 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).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 29, 2024
@mp911de
Copy link
Member

mp911de commented Sep 30, 2024

Care to attach a stack trace of your NPE?

@mp911de mp911de self-assigned this Sep 30, 2024
@sujl95
Copy link
Contributor Author

sujl95 commented Sep 30, 2024

@mp911de

Hello,

As requested, I'm sharing the stack trace where the NullPointerException (NPE) occurs when using the remove() method in RedisMessageListenerContainer. The issue happens when the listener parameter is null, causing the hashCode() method to be invoked on a null key. Please find the attached stack trace screenshot for further details.

If you need any additional information, feel free to let me know. Thank you!
스크린샷 2024-10-01 오전 12 17 29

java.lang.NullPointerException: Cannot invoke "Object.hashCode()" because "key" is null
	at java.base/java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:936)
	at org.springframework.data.redis.listener.RedisMessageListenerContainer.remove(RedisMessageListenerContainer.java:774)
	at org.springframework.data.redis.listener.RedisMessageListenerContainer.removeListener(RedisMessageListenerContainer.java:731)
	at org.springframework.data.redis.listener.RedisMessageListenerContainer.removeMessageListener(RedisMessageListenerContainer.java:562)
	at org.springframework.data.redis.listener.RedisMessageListenerContainer.removeMessageListener(RedisMessageListenerContainer.java:576)

@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 1, 2024
@mp911de mp911de changed the title Update RedisMessageListenerContainer.remove() to handle null listeners gracefully. Correctly handle null listener in RedisMessageListenerContainer.remove(…) Oct 10, 2024
@mp911de mp911de added this to the 3.2.11 (2023.1.11) milestone Oct 10, 2024
@mp911de mp911de closed this in b1453da Oct 10, 2024
mp911de added a commit that referenced this pull request Oct 10, 2024
Simplify code and tests.

See #3009
mp911de pushed a commit that referenced this pull request Oct 10, 2024
mp911de added a commit that referenced this pull request Oct 10, 2024
Simplify code and tests.

See #3009
mp911de pushed a commit that referenced this pull request Oct 10, 2024
mp911de added a commit that referenced this pull request Oct 10, 2024
Simplify code and tests.

See #3009
@mp911de
Copy link
Member

mp911de commented Oct 10, 2024

Thank you for your contribution. That's merged, polished, and backported now.

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

Successfully merging this pull request may close these issues.

3 participants