You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/05/07 10:00:25 UTC
[GitHub] activemq-artemis pull request #2075: ARTEMIS-1852 PageCursorProvider is leak...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/2075
ARTEMIS-1852 PageCursorProvider is leaking cleanup tasks while stopping
PageCursorProviderImpl is not handling any pending cleanup tasks
on stop, leaving paging enabled due to the remaining pages to be
cleared up.
It fixes testTopicsWithNonDurableSubscription.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-1852
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2075.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2075
----
----
---
[GitHub] activemq-artemis pull request #2075: ARTEMIS-1852 PageCursorProvider is leak...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2075#discussion_r186406297
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageCursorProviderImpl.java ---
@@ -239,8 +239,28 @@ public void stop() {
for (PageSubscription cursor : activeCursors.values()) {
cursor.stop();
}
-
- executor.shutdownNow();
+ int pendingCleanupTasks = scheduledCleanup.get();
--- End diff --
Nice catch.. very nice...
However.. I'm not sure why we were calling shutdown here. PageCursor doesn't own the executor...
this is using ArtemisExecutor. where shutdownNow will call an empty method.. and getPendingTasks will return also an empty array.
I think the only thing you should do here is call the flush method.
---
[GitHub] activemq-artemis pull request #2075: ARTEMIS-1852 PageCursorProvider is leak...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2075#discussion_r186412623
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageCursorProviderImpl.java ---
@@ -239,8 +239,28 @@ public void stop() {
for (PageSubscription cursor : activeCursors.values()) {
cursor.stop();
}
-
- executor.shutdownNow();
+ int pendingCleanupTasks = scheduledCleanup.get();
--- End diff --
`PagingStoreImpl` seems the one that is owning the executor
I will move the `executor.shutdownNow` from the `PageCursorProvider` to `PagingStoreImpl` while leaving at least the flush of the cleanup tasks on it :+1:
Good points!
---
[GitHub] activemq-artemis pull request #2075: ARTEMIS-1852 PageCursorProvider is leak...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2075#discussion_r186420454
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageCursorProviderImpl.java ---
@@ -239,8 +239,28 @@ public void stop() {
for (PageSubscription cursor : activeCursors.values()) {
cursor.stop();
}
-
- executor.shutdownNow();
+ int pendingCleanupTasks = scheduledCleanup.get();
--- End diff --
@clebertsuconic How it seems now?
---
[GitHub] activemq-artemis pull request #2075: ARTEMIS-1852 PageCursorProvider is leak...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2075
---
[GitHub] activemq-artemis pull request #2075: ARTEMIS-1852 PageCursorProvider is leak...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2075#discussion_r186410023
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageCursorProviderImpl.java ---
@@ -239,8 +239,28 @@ public void stop() {
for (PageSubscription cursor : activeCursors.values()) {
cursor.stop();
}
-
- executor.shutdownNow();
+ int pendingCleanupTasks = scheduledCleanup.get();
--- End diff --
@clebertsuconic :+1: TBH you're right! So let me check if it already fixes the error and I will move the logic of the pending runnable tasks to be executed on the owner (the `PageStore` I suppose)...wdyt?
---
[GitHub] activemq-artemis pull request #2075: ARTEMIS-1852 PageCursorProvider is leak...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2075#discussion_r186411142
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageCursorProviderImpl.java ---
@@ -239,8 +239,28 @@ public void stop() {
for (PageSubscription cursor : activeCursors.values()) {
cursor.stop();
}
-
- executor.shutdownNow();
+ int pendingCleanupTasks = scheduledCleanup.get();
--- End diff --
It’s probavly the server.
But we need to be careful as wildfly will Inject the pool.
---