You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gaohoward <gi...@git.apache.org> on 2018/03/20 11:05:25 UTC
[GitHub] activemq-artemis pull request #1959: ARTEMIS-1754 LargeServerMessageImpl.toS...
GitHub user gaohoward opened a pull request:
https://github.com/apache/activemq-artemis/pull/1959
ARTEMIS-1754 LargeServerMessageImpl.toString() may leak files
This overridden method calls getPersistentSize() which may lead
to open the large file to get the size. Calling it alone will
cause the file being opened without closing it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaohoward/activemq-artemis g_1754
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1959.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 #1959
----
commit 40fde75d6483b76330292957df7e77313973963a
Author: Howard Gao <ho...@...>
Date: 2018-03-20T11:01:16Z
ARTEMIS-1754 LargeServerMessageImpl.toString() may leak files
This overridden method calls getPersistentSize() which may lead
to open the large file to get the size. Calling it alone will
cause the file being opened without closing it.
----
---
[GitHub] activemq-artemis issue #1959: ARTEMIS-1754 LargeServerMessageImpl.toString()...
Posted by cshannon <gi...@git.apache.org>.
Github user cshannon commented on the issue:
https://github.com/apache/activemq-artemis/pull/1959
Shouldn't we just have the getPersistentSize() method open/close the encoder? The idea is that this method might be called by anyone as it is public so this is a bigger issue than just in the toString call
---
[GitHub] activemq-artemis pull request #1959: ARTEMIS-1754 LargeServerMessageImpl.toS...
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/1959#discussion_r175767855
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/LargeServerMessageImpl.java ---
@@ -347,24 +351,31 @@ public SequentialFile getFile() throws ActiveMQException {
@Override
public long getPersistentSize() throws ActiveMQException {
+ return getPersistentSize(false);
+ }
+
+ private long getPersistentSize(boolean keepFileState) throws ActiveMQException {
long size = super.getPersistentSize();
- size += getBodyEncoder().getLargeBodySize();
+ size += getBodyEncoder(keepFileState).getLargeBodySize();
return size;
}
+
@Override
public String toString() {
try {
- return "LargeServerMessage[messageID=" + messageID + ",durable=" + isDurable() + ",userID=" + getUserID() + ",priority=" + this.getPriority() + ", timestamp=" + toDate(getTimestamp()) + ",expiration=" + toDate(getExpiration()) + ", durable=" + durable + ", address=" + getAddress() + ",size=" + getPersistentSize() + ",properties=" + (properties != null ? properties.toString() : "") + "]@" + System.identityHashCode(this);
+ return "LargeServerMessage[messageID=" + messageID + ",durable=" + isDurable() + ",userID=" + getUserID() + ",priority=" + this.getPriority() + ", timestamp=" + toDate(getTimestamp()) + ",expiration=" + toDate(getExpiration()) + ", durable=" + durable + ", address=" + getAddress() + ",size=" + getPersistentSize(true) + ",properties=" + (properties != null ? properties.toString() : "") + "]@" + System.identityHashCode(this);
--- End diff --
just remove the fileSize from here... and that's the only change you will need.
---
[GitHub] activemq-artemis pull request #1959: ARTEMIS-1754 LargeServerMessageImpl.toS...
Posted by gaohoward <gi...@git.apache.org>.
Github user gaohoward commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1959#discussion_r175771690
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/LargeServerMessageImpl.java ---
@@ -347,24 +351,31 @@ public SequentialFile getFile() throws ActiveMQException {
@Override
public long getPersistentSize() throws ActiveMQException {
+ return getPersistentSize(false);
+ }
+
+ private long getPersistentSize(boolean keepFileState) throws ActiveMQException {
long size = super.getPersistentSize();
- size += getBodyEncoder().getLargeBodySize();
+ size += getBodyEncoder(keepFileState).getLargeBodySize();
return size;
}
+
@Override
public String toString() {
try {
- return "LargeServerMessage[messageID=" + messageID + ",durable=" + isDurable() + ",userID=" + getUserID() + ",priority=" + this.getPriority() + ", timestamp=" + toDate(getTimestamp()) + ",expiration=" + toDate(getExpiration()) + ", durable=" + durable + ", address=" + getAddress() + ",size=" + getPersistentSize() + ",properties=" + (properties != null ? properties.toString() : "") + "]@" + System.identityHashCode(this);
+ return "LargeServerMessage[messageID=" + messageID + ",durable=" + isDurable() + ",userID=" + getUserID() + ",priority=" + this.getPriority() + ", timestamp=" + toDate(getTimestamp()) + ",expiration=" + toDate(getExpiration()) + ", durable=" + durable + ", address=" + getAddress() + ",size=" + getPersistentSize(true) + ",properties=" + (properties != null ? properties.toString() : "") + "]@" + System.identityHashCode(this);
--- End diff --
ok, I'm fine with that.
---
[GitHub] activemq-artemis pull request #1959: ARTEMIS-1754 LargeServerMessageImpl.toS...
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/1959#discussion_r175767737
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/LargeServerMessageImpl.java ---
@@ -376,6 +387,11 @@ public synchronized void validateFile() throws ActiveMQException {
openFile();
bodySize = file.size();
+
+ if (keepFileState) {
--- End diff --
You should have done the opposite...
if you named this variable keep, you should have done if !keepFileState
and you should have sent false when not needed...
if the problem was just the toStringI(), just remove the filesize from the toString()... no need for anything else.
---
[GitHub] activemq-artemis issue #1959: ARTEMIS-1754 LargeServerMessageImpl.toString()...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1959
@cshannon +1.. i will change it
---
[GitHub] activemq-artemis pull request #1959: ARTEMIS-1754 LargeServerMessageImpl.toS...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1959
---