[GitHub] [activemq] thodimi1 commented on issue #227: SSL Connection leaks
thodimi1 commented on issue #227: SSL Connection leaks URL: https://github.com/apache/activemq/pull/227#issuecomment-530627194 qwe This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq] thodimi1 closed pull request #227: SSL Connection leaks
thodimi1 closed pull request #227: SSL Connection leaks URL: https://github.com/apache/activemq/pull/227 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] Havret opened a new pull request #31: AMQNET-612: ObjectMessage shouldn't have jms-msg-type set
Havret opened a new pull request #31: AMQNET-612: ObjectMessage shouldn't have jms-msg-type set URL: https://github.com/apache/activemq-nms-amqp/pull/31 It addresses https://issues.apache.org/jira/browse/AMQNET-612 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] Havret opened a new pull request #30: AMQNET-611: Apache.NMS.IllegalStateException is throw on transport thread
Havret opened a new pull request #30: AMQNET-611: Apache.NMS.IllegalStateException is throw on transport thread URL: https://github.com/apache/activemq-nms-amqp/pull/30 It closes https://issues.apache.org/jira/projects/AMQNET/issues/AMQNET-611 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] Havret edited a comment on issue #28: NO-JIRA: Extend logging
Havret edited a comment on issue #28: NO-JIRA: Extend logging URL: https://github.com/apache/activemq-nms-amqp/pull/28#issuecomment-530553439 @cjwmorgan-sol There weren't any logs there, because in qpid-jms there weren't any logs. I don't see any reason why wouldn't we have them included. The only question is, what level of logging should we use. @michaelandrepearce Any thoughts? BTW. qpid-jms uses tracer to handle this particular situation, but this is something they've introduced just recently. And this is definitely not the same kind of tracer that we have in NMS. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on issue #28: NO-JIRA: Extend logging
Havret commented on issue #28: NO-JIRA: Extend logging URL: https://github.com/apache/activemq-nms-amqp/pull/28#issuecomment-530553439 @cjwmorgan-sol There weren't any logs there, because in qpid-jms there weren't any logs. I don't see any reason why wouldn't we have them included. The only question is, what level of logging should we use. @michaelandrepearce Any thoughts? BTW. qpid-jms uses tracer to handle this particular situation, but this is something they've introduced just recently. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on a change in pull request #28: NO-JIRA: Extend logging
Havret commented on a change in pull request #28: NO-JIRA: Extend logging URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323441669 ## File path: src/NMS.AMQP/Transport/TransportContext.cs ## @@ -56,7 +56,6 @@ static TransportContext() //Warning Warn //Error Error // -Amqp.Trace.TraceLevel = Amqp.TraceLevel.Verbose | Amqp.TraceLevel.Frame; Review comment: Good catch. I disabled them because they bloated my logs during debugging. Taking the opportunity, I change the implementation to adjust it to qpid. From now on frames logging is disabled by default, and you can enable it by adding the option amqp.traceFrames=true to your connection uri. https://qpid.apache.org/releases/qpid-jms-0.29.0/docs/index.html#logging This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on a change in pull request #28: NO-JIRA: Extend logging
Havret commented on a change in pull request #28: NO-JIRA: Extend logging
URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323436467
##
File path: src/NMS.AMQP/Provider/Amqp/AmqpProvider.cs
##
@@ -58,6 +58,9 @@ public Task Connect(ConnectionInfo connectionInfo)
internal void OnConnectionClosed(Error error)
{
+if (Tracer.IsDebugEnabled)
+Tracer.Debug($"Connection closed. {error}");
Review comment:
Sorry. I missed that one.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-nms-amqp] cjwmorgan-sol commented on a change in pull request #28: NO-JIRA: Extend logging
cjwmorgan-sol commented on a change in pull request #28: NO-JIRA: Extend logging
URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323430045
##
File path: src/NMS.AMQP/Provider/Amqp/AmqpProvider.cs
##
@@ -58,6 +58,9 @@ public Task Connect(ConnectionInfo connectionInfo)
internal void OnConnectionClosed(Error error)
{
+if (Tracer.IsDebugEnabled)
+Tracer.Debug($"Connection closed. {error}");
Review comment:
Is it done? I don't see curleys?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on a change in pull request #28: NO-JIRA: Extend logging
Havret commented on a change in pull request #28: NO-JIRA: Extend logging
URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323419221
##
File path: src/NMS.AMQP/Provider/Amqp/AmqpProvider.cs
##
@@ -58,6 +58,9 @@ public Task Connect(ConnectionInfo connectionInfo)
internal void OnConnectionClosed(Error error)
{
+if (Tracer.IsDebugEnabled)
+Tracer.Debug($"Connection closed. {error}");
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on a change in pull request #28: NO-JIRA: Extend logging
Havret commented on a change in pull request #28: NO-JIRA: Extend logging
URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323419156
##
File path: src/NMS.AMQP/NmsSession.cs
##
@@ -470,6 +479,10 @@ public NmsMessageProducer ProducerClosed(Id producerId,
Exception cause)
Tracer.DebugFormat("Ignoring exception thrown during
cleanup of closed producer", error);
}
}
+else
+{
+Tracer.Debug($"NmsMessageProducer: {producerId} not found in
session {this.SessionInfo.Id}.");
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on a change in pull request #28: NO-JIRA: Extend logging
Havret commented on a change in pull request #28: NO-JIRA: Extend logging
URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323418968
##
File path: src/NMS.AMQP/NmsMessageConsumer.cs
##
@@ -219,19 +225,24 @@ private void DeliverNextPending()
{
var envelope = messageQueue.DequeueNoWait();
if (envelope == null)
+{
+if (Tracer.IsDebugEnabled)
+Tracer.Debug($"No message available for
delivery.");
+
return;
+}
if (IsMessageExpired(envelope))
{
if (Tracer.IsDebugEnabled)
-Tracer.Debug($"{Info.Id} filtered expired
message: {envelope.Message.NMSMessageId}");
+Tracer.Error($"{Info.Id} filtered expired
message: {envelope.Message.NMSMessageId}");
DoAckExpired(envelope);
}
else if (IsRedeliveryExceeded(envelope))
{
if (Tracer.IsDebugEnabled)
-Tracer.Debug($"{Info.Id} filtered message
with excessive redelivery count: {envelope.RedeliveryCount}");
+Tracer.Error($"{Info.Id} filtered message
with excessive redelivery count: {envelope.RedeliveryCount}");
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on a change in pull request #28: NO-JIRA: Extend logging
Havret commented on a change in pull request #28: NO-JIRA: Extend logging
URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323418636
##
File path: src/NMS.AMQP/NmsMessageConsumer.cs
##
@@ -219,19 +225,24 @@ private void DeliverNextPending()
{
var envelope = messageQueue.DequeueNoWait();
if (envelope == null)
+{
+if (Tracer.IsDebugEnabled)
+Tracer.Debug($"No message available for
delivery.");
+
return;
+}
if (IsMessageExpired(envelope))
{
if (Tracer.IsDebugEnabled)
-Tracer.Debug($"{Info.Id} filtered expired
message: {envelope.Message.NMSMessageId}");
+Tracer.Error($"{Info.Id} filtered expired
message: {envelope.Message.NMSMessageId}");
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on a change in pull request #28: NO-JIRA: Extend logging
Havret commented on a change in pull request #28: NO-JIRA: Extend logging
URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323418784
##
File path: src/NMS.AMQP/NmsMessageConsumer.cs
##
@@ -219,19 +225,24 @@ private void DeliverNextPending()
{
var envelope = messageQueue.DequeueNoWait();
if (envelope == null)
+{
+if (Tracer.IsDebugEnabled)
+Tracer.Debug($"No message available for
delivery.");
+
return;
+}
if (IsMessageExpired(envelope))
{
if (Tracer.IsDebugEnabled)
-Tracer.Debug($"{Info.Id} filtered expired
message: {envelope.Message.NMSMessageId}");
+Tracer.Error($"{Info.Id} filtered expired
message: {envelope.Message.NMSMessageId}");
DoAckExpired(envelope);
}
else if (IsRedeliveryExceeded(envelope))
{
if (Tracer.IsDebugEnabled)
-Tracer.Debug($"{Info.Id} filtered message
with excessive redelivery count: {envelope.RedeliveryCount}");
+Tracer.Error($"{Info.Id} filtered message
with excessive redelivery count: {envelope.RedeliveryCount}");
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on a change in pull request #28: NO-JIRA: Extend logging
Havret commented on a change in pull request #28: NO-JIRA: Extend logging
URL: https://github.com/apache/activemq-nms-amqp/pull/28#discussion_r323418336
##
File path: src/NMS.AMQP/NmsSession.cs
##
@@ -428,6 +432,11 @@ public void AcknowledgeIndividual(AckType ackType,
InboundMessageDispatch envelo
internal void
EnqueueForDispatch(NmsMessageConsumer.MessageDeliveryTask task)
{
+if (Tracer.IsDebugEnabled)
+{
Review comment:
I kinda like it better with curly brackets.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on issue #29: AMQNET-610: Race conditions during consumer creation
michaelandrepearce commented on issue #29: AMQNET-610: Race conditions during consumer creation URL: https://github.com/apache/activemq-nms-amqp/pull/29#issuecomment-530462233 Ok. Just remember there will always be bugs. We do want to cut a release at somepoint. Once we have a release done we can always do another in a few more weeks. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on issue #29: AMQNET-610: Race conditions during consumer creation
Havret commented on issue #29: AMQNET-610: Race conditions during consumer creation URL: https://github.com/apache/activemq-nms-amqp/pull/29#issuecomment-530458732 Yep. But it would be nice to have [this one](https://issues.apache.org/jira/projects/AMQNET/issues/AMQNET-611) included in the initial release as well. It will be a quick fix though. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on issue #29: AMQNET-610: Race conditions during consumer creation
michaelandrepearce commented on issue #29: AMQNET-610: Race conditions during consumer creation URL: https://github.com/apache/activemq-nms-amqp/pull/29#issuecomment-530456020 Cool was this the only blocker for CI? Or more issues you want to fix. Just trying to gauge when to cut release This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] michaelandrepearce merged pull request #29: AMQNET-610: Race conditions during consumer creation
michaelandrepearce merged pull request #29: AMQNET-610: Race conditions during consumer creation URL: https://github.com/apache/activemq-nms-amqp/pull/29 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] Havret commented on issue #29: AMQNET-610: Race conditions during consumer creation
Havret commented on issue #29: AMQNET-610: Race conditions during consumer creation URL: https://github.com/apache/activemq-nms-amqp/pull/29#issuecomment-530447174 I'm going to look at it tonight. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] michaelandrepearce commented on issue #29: AMQNET-610: Race conditions during consumer creation
michaelandrepearce commented on issue #29: AMQNET-610: Race conditions during consumer creation URL: https://github.com/apache/activemq-nms-amqp/pull/29#issuecomment-530446482 @HavretGC did we resolve the issue robbie raised in dev mail list re object msg This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] asfgit closed pull request #2831: ARTEMIS-2480 - Reloading configuration can kill broker
asfgit closed pull request #2831: ARTEMIS-2480 - Reloading configuration can kill broker URL: https://github.com/apache/activemq-artemis/pull/2831 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] HavretGC commented on issue #29: AMQNET-610: Race conditions during consumer creation
HavretGC commented on issue #29: AMQNET-610: Race conditions during consumer creation URL: https://github.com/apache/activemq-nms-amqp/pull/29#issuecomment-530422019 @michaelandrepearce It is ready to merge then. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-nms-amqp] cjwmorgan-sol commented on issue #29: AMQNET-610: Race conditions during consumer creation
cjwmorgan-sol commented on issue #29: AMQNET-610: Race conditions during consumer creation URL: https://github.com/apache/activemq-nms-amqp/pull/29#issuecomment-530393632 Raised https://issues.apache.org/jira/browse/AMQNET-611, this PR looks fine to me. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] asfgit closed pull request #2828: ARTEMIS-2478 Expired message not removed in non destructive queue
asfgit closed pull request #2828: ARTEMIS-2478 Expired message not removed in non destructive queue URL: https://github.com/apache/activemq-artemis/pull/2828 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2828: ARTEMIS-2478 Expired message not removed in non destructive queue
clebertsuconic commented on issue #2828: ARTEMIS-2478 Expired message not removed in non destructive queue URL: https://github.com/apache/activemq-artemis/pull/2828#issuecomment-530373259 Actually, there's one issue (it seems the test added takes a long time to run.. 1 minute). I'm looking if we can speed it up before merging.. i will get back to you on that. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2828: ARTEMIS-2478 Expired message not removed in non destructive queue
clebertsuconic commented on issue #2828: ARTEMIS-2478 Expired message not removed in non destructive queue URL: https://github.com/apache/activemq-artemis/pull/2828#issuecomment-530371857 I ran the whole testsuite (2.5 hours run) with this change, no issues! merging! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530319911 @wy96f I've taken a second look to the way Netty handle thread local `ByteBuf`pools and I can confirm that `-Dio.netty.allocator.useCacheForAllThreads=false` should save non-netty threads from creating such thread local regions for tiny/small/medium sized buffers. While related to > but they just contain wrappers to direct memory that cannot be released (is part of the pool and they have no cleaner). The impact should be way less then IOUtil that pool native ByteBuffers holding exclusively native memory that won't be reused anymore... I have to add more detail and I was wrong: the tiny/small/medium thread local caches are actually leaking memory regions (with default `io.netty.allocator.useCacheForAllThread`) that need the holding thread to die in order to make them available to other threads again, but their size is quite small by default and the size I've chosen as `LARGE_MESSAGE_CHUNK_SIZE` (ie 100 * 1024) is `medium` but doesn't fall in any of those caches, so no real leak will happen until we allocate something smaller (could happen but is rare). IMO we should consider 2 separate improvements for a separate PR: - allow threads to stay alive forever and in a fixed number, but just idle (and maybe using the FJ thread pool executor instead of the AMQ thread pool executor in that case): in that case we can consider such caches to be ok to be used by *all* threads (including non-netty ones), because the leak is just to cope with future load - for the AMQ thread pool executor as it is, make the presence of such caches configurable and off by default wdyt? Thanks for the comment, good catch! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530319911 @wy96f I've taken a second look to the way Netty handle thread local `ByteBuf`pools and I can confirm that `-Dio.netty.allocator.useCacheForAllThreads=false` should save non-netty threads from creating such thread local regions for tiny/small/medium sized buffers. While related to > but they just contain wrappers to direct memory that cannot be released (is part of the pool and they have no cleaner). The impact should be way less then IOUtil that pool native ByteBuffers holding exclusively native memory that won't be reused anymore... I have to add more detail and I was wrong: the tiny/small/medium thread local caches are actually leaking memory regions (with default `io.netty.allocator.useCacheForAllThread`) that need the holding thread to die in order to make them available to other threads again, but their size is quite small by default and the size I've chosen as `LARGE_MESSAGE_CHUNK_SIZE` (ie 100 * 1024) is `medium` but doesn't fall in any of those caches, so no real leak will happen until we allocate something smaller (could happen but is rare). IMO we should consider 2 separate improvements for a separate PR: - allow threads to stay alive forever and in a fixed number, but just idle (and maybe using the FJ thread pool executor instead of the AMQ thread pool executor in that case): with this we can consider such caches to be ok to be used by *all* threads (including non-netty ones), because the leak is just to cope with future load and they won't be deallocated just due to inactivity - for the AMQ thread pool executor as it is, make the presence of such caches configurable and off by default wdyt? Thanks for the comment, good catch! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530319911 @wy96f I've taken a second look to the way Netty handle thread local `ByteBuf`pools and I can confirm that `-Dio.netty.allocator.useCacheForAllThreads=false` should save non-netty threads from creating such thread local regions for tiny/small/medium sized buffers. While related to > but they just contain wrappers to direct memory that cannot be released (is part of the pool and they have no cleaner). The impact should be way less then IOUtil that pool native ByteBuffers holding exclusively native memory that won't be reused anymore... I have to add more detail and I was wrong: the tiny/small/medium thread local caches are actually leaking memory regions (with default `io.netty.allocator.useCacheForAllThread`) that need the holding thread to die in order to make them available to other threads again, but their size is quite small by default and the size I've chosen as `LARGE_MESSAGE_CHUNK_SIZE` (ie 100 * 1024) is `medium` but doesn't fall in any of those caches, so no real leak will happen until we allocate something smaller (could happen but is rare). IMO we should consider 2 separate improvements for a separate PR: - allow threads to stay alive forever, but just idle (and maybe using the FJ thread pool executor instead of the AMQ thread pool executor in that case): in that case we can consider such caches to be ok to be used by *all* threads (including non-netty ones), because the leak is just to cope with future load - for the AMQ thread pool executor as it is, make the presence of such caches configurable and off by default wdyt? Thanks for the comment, good catch! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530319911 @wy96f I've taken a second look to the way Netty handle thread local pools and I can confirm that `-Dio.netty.allocator.useCacheForAllThreads=false` should save non-netty threads from creating such thread local regions for tiny/small/medium sized buffers. While related to > but they just contain wrappers to direct memory that cannot be released (is part of the pool and they have no cleaner). The impact should be way less then IOUtil that pool native ByteBuffers holding exclusively native memory that won't be reused anymore... I have to add more detail and I was wrong: the tiny/small/medium thread local caches are actually leaking memory regions (with default `io.netty.allocator.useCacheForAllThread`) that need the holding thread to die in order to make them available to other threads again, but their size is quite small by default and the size I've chosen as `LARGE_MESSAGE_CHUNK_SIZE` (ie 100 * 1024) is `medium` but doesn't fall in any of those caches, so no real leak will happen until we allocate something smaller (could happen but is rare). IMO we should consider 2 separate improvements for a separate PR: - allow threads to stay alive forever, but just idle (and maybe using the FJ thread pool executor instead of the AMQ thread pool executor in that case): in that case we can consider such caches to be ok to be used by *all* threads (including non-netty ones), because the leak is just to cope with future load - for the AMQ thread pool executor as it is, make the presence of such caches configurable and off by default wdyt? Thanks for the comment, good catch! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530319911 @wy96f I've taken a second look to the way Netty handle thread local pools and I can confirm that `-Dio.netty.allocator.useCacheForAllThreads=false` should save non-netty threads from creating such thread local regions for tiny/small/medium sized buffers. While related to > but they just contain wrappers to direct memory that cannot be released (is part of the pool and they have no cleaner). The impact should be way less then IOUtil that pool native ByteBuffers holding exclusively native memory that won't be reused anymore... I have to add more detail and I was wrong: the tiny/small/medium thread local caches are actually leaking memory regions (with default `io.netty.allocator.useCacheForAllThread`) that need the holding thread to die in order to make them available to other threads again, but their size is quite small by default and the size I've chosen as `LARGE_MESSAGE_CHUNK_SIZE` (ie 100 * 1024) is `medium` but doesn't fall in any of those caches, so no real leak will happen until we allocate something smaller (could happen but is rare). IMO we should consider 2 separate improvements for a separate PR: - allow threads to stay alive forever, but just idle (and maybe using the FJ thread pool executor instead of the AMQ thread pool executor in that case): in that case we can consider such caches to be ok to be used by *all* threads (including non-netty ones), because the leak is just to cope with future load - for the AMQ thread pool executor as it is, make the presence of such caches configurable and off by default wdyt? Thanks for the comment, good catch! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530319911 @wy96f I've taken a second look to the way Netty handle thread local pools and I can confirm that `-Dio.netty.allocator.useCacheForAllThreads=false` should save non-netty threads from creating such thread local regions for tiny/small/medium sized buffers. While related to > but they just contain wrappers to direct memory that cannot be released (is part of the pool and they have no cleaner). The impact should be way less then IOUtil that pool native ByteBuffers holding exclusively native memory that won't be reused anymore... I have to add more detail and I was wrong: the tiny/small/medium thread local caches are actually leaking memory regions (with default `io.netty.allocator.useCacheForAllThread`) that need the holding thread to die in order to make them available to other threads again, but their size is quite small by default and the size I've chosen as `LARGE_MESSAGE_CHUNK_SIZE` (ie 100 * 1024) is `medium` but doesn't fall in any of those caches, so no real leak will happen until we allocate something smaller (could happen but is rare). IMO we should consider (for a separate issue) 2 separate improvements: - allow threads to stay alive forever, but just idle (and maybe using the FJ thread pool executor instead of the AMQ thread pool executor in that case): in that case we can consider such caches to be ok to be used by *all* threads (including non-netty ones), because the leak is just to cope with future load - for the AMQ thread pool executor as it is, make the presence of such caches configurable and off by default wdyt? Thanks for the comment, good catch! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] andytaylor commented on issue #2831: ARTEMIS-2480 - Reloading configuration can kill broker
andytaylor commented on issue #2831: ARTEMIS-2480 - Reloading configuration can kill broker URL: https://github.com/apache/activemq-artemis/pull/2831#issuecomment-530319715 feedback applied This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530319911 @wy96f I've taken a second look to the way Netty handle thread local pools and I can confirm that `-Dio.netty.allocator.useCacheForAllThreads=false` should save non-netty threads from creating such thread local regions for tiny/small/medium sized buffers. While related to > but they just contain wrappers to direct memory that cannot be released (is part of the pool and they have no cleaner). The impact should be way less then IOUtil that pool native ByteBuffers holding exclusively native memory that won't be reused anymore... I have to add more detail and I was wrong: the tiny/small/medium thread local caches are actually leaking memory regions (with default `io.netty.allocator.useCacheForAllThread`) that need the holding thread to die in order to make the available to other threads, but their size is quite small by default and the size I've chosen as `LARGE_MESSAGE_CHUNK_SIZE` (ie 100 * 1024) is `medium` but doesn't fall in any of those caches, so no real leak will happen until we allocate something smaller (could happen but is rare). IMO we should consider (for a separate issue) 2 separate improvements: - allow threads to stay alive forever, but just idle (and maybe using the FJ thread pool executor instead of the AMQ thread pool executor in that case): in that case we can consider such caches to be ok to be used by *all* threads (including non-netty ones), because the leak is just to cope with future load - for the AMQ thread pool executor as it is, make the presence of such caches configurable and off by default wdyt? Thanks for the comment, good catch! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could
leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944
@wy96f Yes and no :)
They would leak (at first look, but let me take a better look), but they
just contain wrappers to direct memory that cannot be released (is part of the
pool and they have no cleaner). The impact should be way less then `IOUtil`
that pool native `ByteBuffer`s holding exclusively native memory that won't be
reused anymore...
Ideally we shouldn't have *any* thread locals, but we use them for several
stuff (including the NIO factories and page read/write): I'm planning to
replaced them by using the Netty pool, because of the difference on how thread
locals are used.
Our thread locals in Artemis just hold memory that cannot be reused if not
by the same thread, while for Netty, thread locals are just a way to cache
wrappers that has a very limited impact on GC, while the pool really allows to
reuse memory where is most needed...
Just to add more detail: consider that
`PooledBytebufAllocator.PoolThreadLocalCache::initialValue` contains this
comment, while creating a thread-local `PoolThreadCache`:
```
@Override
protected synchronized PoolThreadCache initialValue() {
final PoolArena heapArena = leastUsedArena(heapArenas);
final PoolArena directArena =
leastUsedArena(directArenas);
Thread current = Thread.currentThread();
if (useCacheForAllThreads || current instanceof
FastThreadLocalThread) {
return new PoolThreadCache(
heapArena, directArena, tinyCacheSize,
smallCacheSize, normalCacheSize,
DEFAULT_MAX_CACHED_BUFFER_CAPACITY,
DEFAULT_CACHE_TRIM_INTERVAL);
}
// No caching so just use 0 as sizes.
return new PoolThreadCache(heapArena, directArena, 0, 0, 0, 0,
0);
}
```
So, if we are not very happy about the Netty caching on non-Netty threads,
we can disable those caches by setting
`-Dio.netty.allocator.useCacheForAllThreads=false`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could
leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944
@wy96f Yes and no :)
They would leak (at first look, but let me take a better look), but they
just contain wrappers to direct memory that cannot be released (is part of the
pool and they have no cleaner). The impact should be way less then `IOUtil`
that pool native `ByteBuffer`s holding exclusively native memory that won't be
reused anymore...
Ideally we shouldn't have *any* thread locals, but we use them for several
stuff (including the NIO factories and page read/write): I'm planning to
replaced them by using the Netty pool, because of the difference on how thread
locals are used.
Our thread locals in Artemis just hold memory that cannot be reused if not
by the same thread, while for Netty, thread locals are just a way to cache
wrappers that has a very limited impact on GC, while the pool really allows to
reuse memory where is most needed...
Just to add more detail: consider that
`PooledBytebufAllocator.PoolThreadLocalCache::initialValue` contains this
comment, while creating a thread-local `PoolThreadCache`:
```
@Override
protected synchronized PoolThreadCache initialValue() {
final PoolArena heapArena = leastUsedArena(heapArenas);
final PoolArena directArena =
leastUsedArena(directArenas);
Thread current = Thread.currentThread();
if (useCacheForAllThreads || current instanceof
FastThreadLocalThread) {
return new PoolThreadCache(
heapArena, directArena, tinyCacheSize,
smallCacheSize, normalCacheSize,
DEFAULT_MAX_CACHED_BUFFER_CAPACITY,
DEFAULT_CACHE_TRIM_INTERVAL);
}
// No caching so just use 0 as sizes.
return new PoolThreadCache(heapArena, directArena, 0, 0, 0, 0,
0);
}
```
So, if we are not very happy about the Netty caching on non-Netty threads,
we can disable those caches on by setting
`-Dio.netty.allocator.useCacheForAllThreads=false`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could
leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944
@wy96f Yes and no :)
They would leak (at first look, but let me take a better look), but they
just contain wrappers to direct memory that cannot be released (is part of the
pool and they have no cleaner). The impact should be way less then `IOUtil`
that pool native `ByteBuffer`s holding exclusively native memory that won't be
reused anymore...
Ideally we shouldn't have *any* thread locals, but we use them for several
stuff (including the NIO factories and page read/write): I'm planning to
replaced them by using the Netty pool, because of the difference on how thread
locals are used.
Our thread locals in Artemis just hold memory that cannot be reused if not
by the same thread, while for Netty, thread locals are just a way to cache
wrappers that has a very limited impact on GC, while the pool really allows to
reuse memory where is most needed...
Just to add more detail: consider that
`PooledBytebufAllocator.PoolThreadLocalCache::initialValue` contains this
comment, while creating a thread-local `PoolThreadCache`:
```
@Override
protected synchronized PoolThreadCache initialValue() {
final PoolArena heapArena = leastUsedArena(heapArenas);
final PoolArena directArena =
leastUsedArena(directArenas);
Thread current = Thread.currentThread();
if (useCacheForAllThreads || current instanceof
FastThreadLocalThread) {
return new PoolThreadCache(
heapArena, directArena, tinyCacheSize,
smallCacheSize, normalCacheSize,
DEFAULT_MAX_CACHED_BUFFER_CAPACITY,
DEFAULT_CACHE_TRIM_INTERVAL);
}
// No caching so just use 0 as sizes.
return new PoolThreadCache(heapArena, directArena, 0, 0, 0, 0,
0);
}
```
So, if we are not very happy about the Netty caching on non-Netty threads,
we can disable those caches on by setting
`-Dio.netty.allocator.useCacheForAllThreads=false`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944 @wy96f Yes and no :) They would leak (at first look, but let me take a better look), but they just contain wrappers to direct memory that cannot be released (is part of the pool and they have no cleaner). The impact should be way less then `IOUtil` that pool native `ByteBuffer`s holding exclusively native memory that won't be reused anymore... Ideally we shouldn't have *any* thread locals, but we use them for several stuff (including the NIO factories and page read/write): I'm planning to replaced them by using the Netty pool, because of the difference on how thread locals are used. Our thread locals in Artemis just hold memory that cannot be reused if not by the same thread, while for Netty, thread locals are just a way to cache wrappers that has a very limited impact on GC, while the pool really allows to reuse memory where is most needed... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] wy96f edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
wy96f edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530283213 > @wy96f > > > PooledByteBufAllocator::directBuffer uses java's ThreadLocal. Wouldn't this result in leak in the case if thread terminates due to idle timeout as in #2199 ? > > Do you mean the thread local arena? Or the thread local NIO ByteBuffer? > Anyway, at the end of its usage we always release the `ByteBuf` and Netty will take care to release any referenced resources: it's a pool so it leaks "by definition" I mean tinySubPageDirectCaches/smallSubPageDirectCaches/normalDirectCaches in PoolThreadCache. Memory would be added into these caches first if released(not yet into shared arena). Given PoolThreadCache is thread local, would theses caches leak if thread terminates? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944 @wy96f Yes and no :) They would leak (at first look, but let me take a better look), but they just contain wrappers to direct memory that cannot be released (is part of the pool). The impact should be way less then `IOUtil` that pool native `ByteBuffer`s holding exclusively native memory that won't be reused anymore... Ideally we shouldn't have *any* thread locals, but we use them for several stuff (including the NIO factories and page read/write): I'm planning to replaced them by using the Netty pool, because of the difference on how thread locals are used. Our thread locals in Artemis just hold memory that cannot be reused if not by the same thread, while for Netty, thread locals are just a way to cache wrappers that has a very limited impact on GC, while the pool really allows to reuse memory where is most needed... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944 @wy96f Yes and no :) They would leak (at first look, but let me take a better look), but they just contain wrappers to direct memory that cannot be released (is part of the pool). The impact should be way less then `IOUtil` that pool native `ByteBuffer`s holding exclusively native memory that won't be reused anymore... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944 @wy96f Yes and no :) They would leak (at first look, but let me take a better look), but they just contain wrappers to direct memory that cannot be released (is part of the pool) and no cleaner (so they won't collect anything, when contains ByteBuffers). The impact should be way less then `IOUtil` that pool native `ByteBuffer`s holding exclusively native memory that won't be reused anymore... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944 @wy96f Yes and no :) The would leak (at first look, but let me take a better look), but they would contain just wrappers to direct memory that cannot be released (is part of the pool) and no cleaner (so they won't collect anything, when contains ByteBuffers). The impact should be way less then `IOUtil` that pool native `ByteBuffer`s holding exclusively native memory that won't be reused anymore... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530284944 @wy96f Yes and no :) They would leak (at first look, but let me take a better look), but they would contain just wrappers to direct memory that cannot be released (is part of the pool) and no cleaner (so they won't collect anything, when contains ByteBuffers). The impact should be way less then `IOUtil` that pool native `ByteBuffer`s holding exclusively native memory that won't be reused anymore... This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
wy96f commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530283213 > @wy96f > > > PooledByteBufAllocator::directBuffer uses java's ThreadLocal. Wouldn't this result in leak in the case if thread terminates due to idle timeout as in #2199 ? > > Do you mean the thread local arena? Or the thread local NIO ByteBuffer? > Anyway, at the end of its usage we always release the `ByteBuf` and Netty will take care to release any referenced resources: it's a pool so it leaks "by definition" I mean tinySubPageDirectCaches/smallSubPageDirectCaches/normalDirectCaches in PoolThreadCache. PoolChunk would be added into these caches first if released(not yet into shared arena). Given PoolThreadCache is thread local, would theses caches leak if thread terminates? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 commented on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530273041 @wy96f > PooledByteBufAllocator::directBuffer uses java's ThreadLocal. Wouldn't this result in leak in the case if thread terminates due to idle timeout as in #2199 ? Do you mean the thread local arena? Or the thread local NIO ByteBuffer? Anyway, at the end of its usage we always release the `ByteBuf` and Netty will take care to release any referenced resources: it's a pool so it leaks "by definition" :+1: This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 commented on a change in pull request #2832: ARTEMIS-2482 Large
messages could leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#discussion_r323108903
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java
##
@@ -832,46 +835,117 @@ public void stopReplication() {
}
}
+ static final int LARGE_MESSAGE_CHUNK_SIZE = 100 * 1024;
+
+ private void addBytesUsingTempNativeBuffer(final SequentialFile file, final
ActiveMQBuffer bytes) throws Exception {
+ assert file instanceof NIOSequentialFile;
+ //we can't use the actual content of it as it is and need to perform a
copy into a direct ByteBuffer
+ int readableBytes = bytes.readableBytes();
+ final int requiredCapacity = Math.min(LARGE_MESSAGE_CHUNK_SIZE,
readableBytes);
+ final ByteBuf tempBuffer =
PooledByteBufAllocator.DEFAULT.directBuffer(requiredCapacity, requiredCapacity);
+ try {
+ int readerIndex = bytes.readerIndex();
+ while (readableBytes > 0) {
+final int size = Math.min(readableBytes, LARGE_MESSAGE_CHUNK_SIZE);
+final ByteBuffer nioBytes = tempBuffer.internalNioBuffer(0, size);
+final int position = nioBytes.position();
+bytes.getBytes(readerIndex, nioBytes);
+nioBytes.position(position);
+file.blockingWriteDirect(nioBytes, false, false);
+readerIndex += size;
+readableBytes -= size;
+ }
+ } finally {
+ tempBuffer.release();
+ }
+ }
+
public final void addBytesToLargeMessage(final SequentialFile file,
final long messageId,
final ActiveMQBuffer bytes) throws
Exception {
readLock();
try {
file.position(file.size());
if (bytes.byteBuf() != null && bytes.byteBuf().nioBufferCount() == 1)
{
-final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
-file.blockingWriteDirect(nioBytes, false, false);
-
-if (isReplicated()) {
- //copy defensively bytes
- final byte[] bytesCopy = new byte[bytes.readableBytes()];
- bytes.getBytes(bytes.readerIndex(), bytesCopy);
- replicator.largeMessageWrite(messageId, bytesCopy);
+//NIO -> need direct ByteBuffers, while JDBC the opposite
+if (file instanceof NIOSequentialFile) {
+ if (bytes.byteBuf().isDirect()) {
+ final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
+ file.blockingWriteDirect(nioBytes, false, false);
+ } else {
+ addBytesUsingTempNativeBuffer(file, bytes);
+ }
+} else if (!bytes.byteBuf().isDirect()) {
+ final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
Review comment:
it is on
https://github.com/apache/activemq-artemis/pull/2832/files/4fc608a382d9d1d7eb0401e808141c7f939b21f9#diff-6c51f3299aaf6c901fe05c48fcfa3bb7R869
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 commented on a change in pull request #2832: ARTEMIS-2482 Large
messages could leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#discussion_r323108156
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java
##
@@ -832,46 +835,117 @@ public void stopReplication() {
}
}
+ static final int LARGE_MESSAGE_CHUNK_SIZE = 100 * 1024;
+
+ private void addBytesUsingTempNativeBuffer(final SequentialFile file, final
ActiveMQBuffer bytes) throws Exception {
+ assert file instanceof NIOSequentialFile;
+ //we can't use the actual content of it as it is and need to perform a
copy into a direct ByteBuffer
+ int readableBytes = bytes.readableBytes();
+ final int requiredCapacity = Math.min(LARGE_MESSAGE_CHUNK_SIZE,
readableBytes);
+ final ByteBuf tempBuffer =
PooledByteBufAllocator.DEFAULT.directBuffer(requiredCapacity, requiredCapacity);
+ try {
+ int readerIndex = bytes.readerIndex();
+ while (readableBytes > 0) {
+final int size = Math.min(readableBytes, LARGE_MESSAGE_CHUNK_SIZE);
+final ByteBuffer nioBytes = tempBuffer.internalNioBuffer(0, size);
+final int position = nioBytes.position();
+bytes.getBytes(readerIndex, nioBytes);
+nioBytes.position(position);
+file.blockingWriteDirect(nioBytes, false, false);
+readerIndex += size;
+readableBytes -= size;
+ }
+ } finally {
+ tempBuffer.release();
+ }
+ }
+
public final void addBytesToLargeMessage(final SequentialFile file,
final long messageId,
final ActiveMQBuffer bytes) throws
Exception {
readLock();
try {
file.position(file.size());
if (bytes.byteBuf() != null && bytes.byteBuf().nioBufferCount() == 1)
{
-final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
-file.blockingWriteDirect(nioBytes, false, false);
-
-if (isReplicated()) {
- //copy defensively bytes
- final byte[] bytesCopy = new byte[bytes.readableBytes()];
- bytes.getBytes(bytes.readerIndex(), bytesCopy);
- replicator.largeMessageWrite(messageId, bytesCopy);
+//NIO -> need direct ByteBuffers, while JDBC the opposite
+if (file instanceof NIOSequentialFile) {
+ if (bytes.byteBuf().isDirect()) {
+ final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
Review comment:
it is on
https://github.com/apache/activemq-artemis/pull/2832/files/4fc608a382d9d1d7eb0401e808141c7f939b21f9#diff-6c51f3299aaf6c901fe05c48fcfa3bb7R869
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on a change in pull request #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
wy96f commented on a change in pull request #2832: ARTEMIS-2482 Large messages
could leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#discussion_r323071740
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java
##
@@ -832,46 +835,117 @@ public void stopReplication() {
}
}
+ static final int LARGE_MESSAGE_CHUNK_SIZE = 100 * 1024;
+
+ private void addBytesUsingTempNativeBuffer(final SequentialFile file, final
ActiveMQBuffer bytes) throws Exception {
+ assert file instanceof NIOSequentialFile;
+ //we can't use the actual content of it as it is and need to perform a
copy into a direct ByteBuffer
+ int readableBytes = bytes.readableBytes();
+ final int requiredCapacity = Math.min(LARGE_MESSAGE_CHUNK_SIZE,
readableBytes);
+ final ByteBuf tempBuffer =
PooledByteBufAllocator.DEFAULT.directBuffer(requiredCapacity, requiredCapacity);
+ try {
+ int readerIndex = bytes.readerIndex();
+ while (readableBytes > 0) {
+final int size = Math.min(readableBytes, LARGE_MESSAGE_CHUNK_SIZE);
+final ByteBuffer nioBytes = tempBuffer.internalNioBuffer(0, size);
+final int position = nioBytes.position();
+bytes.getBytes(readerIndex, nioBytes);
+nioBytes.position(position);
+file.blockingWriteDirect(nioBytes, false, false);
+readerIndex += size;
+readableBytes -= size;
+ }
+ } finally {
+ tempBuffer.release();
+ }
+ }
+
public final void addBytesToLargeMessage(final SequentialFile file,
final long messageId,
final ActiveMQBuffer bytes) throws
Exception {
readLock();
try {
file.position(file.size());
if (bytes.byteBuf() != null && bytes.byteBuf().nioBufferCount() == 1)
{
-final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
-file.blockingWriteDirect(nioBytes, false, false);
-
-if (isReplicated()) {
- //copy defensively bytes
- final byte[] bytesCopy = new byte[bytes.readableBytes()];
- bytes.getBytes(bytes.readerIndex(), bytesCopy);
- replicator.largeMessageWrite(messageId, bytesCopy);
+//NIO -> need direct ByteBuffers, while JDBC the opposite
+if (file instanceof NIOSequentialFile) {
+ if (bytes.byteBuf().isDirect()) {
+ final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
+ file.blockingWriteDirect(nioBytes, false, false);
+ } else {
+ addBytesUsingTempNativeBuffer(file, bytes);
+ }
+} else if (!bytes.byteBuf().isDirect()) {
+ final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
Review comment:
Do we need to judge nioBufferCount() == 1 first?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] wy96f commented on a change in pull request #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
wy96f commented on a change in pull request #2832: ARTEMIS-2482 Large messages
could leak native ByteBuffers
URL: https://github.com/apache/activemq-artemis/pull/2832#discussion_r323071598
##
File path:
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/JournalStorageManager.java
##
@@ -832,46 +835,117 @@ public void stopReplication() {
}
}
+ static final int LARGE_MESSAGE_CHUNK_SIZE = 100 * 1024;
+
+ private void addBytesUsingTempNativeBuffer(final SequentialFile file, final
ActiveMQBuffer bytes) throws Exception {
+ assert file instanceof NIOSequentialFile;
+ //we can't use the actual content of it as it is and need to perform a
copy into a direct ByteBuffer
+ int readableBytes = bytes.readableBytes();
+ final int requiredCapacity = Math.min(LARGE_MESSAGE_CHUNK_SIZE,
readableBytes);
+ final ByteBuf tempBuffer =
PooledByteBufAllocator.DEFAULT.directBuffer(requiredCapacity, requiredCapacity);
+ try {
+ int readerIndex = bytes.readerIndex();
+ while (readableBytes > 0) {
+final int size = Math.min(readableBytes, LARGE_MESSAGE_CHUNK_SIZE);
+final ByteBuffer nioBytes = tempBuffer.internalNioBuffer(0, size);
+final int position = nioBytes.position();
+bytes.getBytes(readerIndex, nioBytes);
+nioBytes.position(position);
+file.blockingWriteDirect(nioBytes, false, false);
+readerIndex += size;
+readableBytes -= size;
+ }
+ } finally {
+ tempBuffer.release();
+ }
+ }
+
public final void addBytesToLargeMessage(final SequentialFile file,
final long messageId,
final ActiveMQBuffer bytes) throws
Exception {
readLock();
try {
file.position(file.size());
if (bytes.byteBuf() != null && bytes.byteBuf().nioBufferCount() == 1)
{
-final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
-file.blockingWriteDirect(nioBytes, false, false);
-
-if (isReplicated()) {
- //copy defensively bytes
- final byte[] bytesCopy = new byte[bytes.readableBytes()];
- bytes.getBytes(bytes.readerIndex(), bytesCopy);
- replicator.largeMessageWrite(messageId, bytesCopy);
+//NIO -> need direct ByteBuffers, while JDBC the opposite
+if (file instanceof NIOSequentialFile) {
+ if (bytes.byteBuf().isDirect()) {
+ final ByteBuffer nioBytes =
bytes.byteBuf().internalNioBuffer(bytes.readerIndex(), bytes.readableBytes());
Review comment:
Do we need to judge nioBufferCount() == 1 first?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers
franz1981 edited a comment on issue #2832: ARTEMIS-2482 Large messages could leak native ByteBuffers URL: https://github.com/apache/activemq-artemis/pull/2832#issuecomment-530138441 @wy96f please take a look: I know you've recently have fun with `ByteBuf`s :) I see too that the change introduced with 76d420590fa73aefb41713a5589dcec22588c594 has a memory leak similar to the one of this PR on https://github.com/apache/activemq-artemis/blob/e537fbfde06a5a09ef369401e715970c4003bd32/artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/Page.java#L149 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
