Skip to content

GH-3785: Close stream for persistent collection #3786

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

Merged
merged 2 commits into from
Apr 25, 2022

Conversation

artembilan
Copy link
Member

Fixes #3785

  • Fix CollectionArgumentResolver and PayloadsArgumentResolver to
    close the Stream of message after its usage
  • Rework AbstractKeyValueMessageStore.removeMessagesFromGroup()
    to iterate input collection of messages not its stream to avoid
    the mentioned problem

Cherry-pick to 5.5.x

Fixes spring-projects#3785

* Fix `CollectionArgumentResolver` and `PayloadsArgumentResolver` to
close the `Stream` of message after its usage
* Rework `AbstractKeyValueMessageStore.removeMessagesFromGroup()`
to iterate input collection of messages not its stream to avoid
the mentioned problem

**Cherry-pick to `5.5.x`**
@garyrussell
Copy link
Contributor

No tests needed?

@artembilan
Copy link
Member Author

Well, it doesn't look obvious what test has to do, but I'll think about that tomorrow.
For self-learning purpose.
I just don't want to add more deps for no extreme reason....

@garyrussell
Copy link
Contributor

Maybe a mock AMGS with a message stream passed into each of the resolvers and verify that the mock stream is closed?

Perhaps we also should enhance the javadocs of the JDBC MGS to say that the stream must be closed; the AGMS just streams the getMessages() so close is not required there, but any subclass that uses true streaming presumably needs the stream closed.

It looks like we don't even have any unit tests for PMG.streamMessages().

…ak()`

to ensure that no leaks in the connection pool anymore.
* Improve `MessageGroupStore.streamMessagesForGroup()` JavaDocs about
requirements to close the `Stream` from persistent message store impls
@garyrussell garyrussell merged commit e454f59 into spring-projects:main Apr 25, 2022
garyrussell pushed a commit that referenced this pull request Apr 25, 2022
* GH-3785: Close stream for persistent collection

Fixes #3785

* Fix `CollectionArgumentResolver` and `PayloadsArgumentResolver` to
close the `Stream` of message after its usage
* Rework `AbstractKeyValueMessageStore.removeMessagesFromGroup()`
to iterate input collection of messages not its stream to avoid
the mentioned problem

**Cherry-pick to `5.5.x`**

* * Add `JdbcMessageStoreTests.testMessageGroupStreamNoConnectionPoolLeak()`
to ensure that no leaks in the connection pool anymore.
* Improve `MessageGroupStore.streamMessagesForGroup()` JavaDocs about
requirements to close the `Stream` from persistent message store impls
@garyrussell
Copy link
Contributor

... and cherry-picked.

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 this pull request may close these issues.

JdbcMessageStore doesn't close database connections after streaming stored messages
2 participants