You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/02/11 14:53:43 UTC
[GitHub] [activemq-artemis] BBE78 opened a new pull request #3952: Fix for ARTEMIS-3509
BBE78 opened a new pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952
Concurrent modifications happened sometimes on non synchronized methods that use "refsToAck" list
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1036505405
a test would be nice
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1037388502
Using `CopyOnWriteArrayList` is better than using `synchronized` relatively speaking, but I'm still not in favor of a change that is going to have such a widespread impact for an issue that seems confined to the STOMP implementation. As noted previously, I recommend you track down the root cause of the concurrent access and add protection there.
As far as testing goes. I will typically write a system/integration test that brute forces the issue to occur. For example, if there's a race condition in connection handling then I'll spin up a few hundred threads and create as many connections as possible as quickly as possible. Once I can brute force the issue semi-reliably then I'll gather logging to narrow down the issue until the root cause is clear at which point a proper fix can be 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1036432301
I don't think adding `synchronized` to these methods is a valid fix. This is going to impose a potentially significant performance penalty across the broker for just about any transaction. Also, you have not included a test to validate the fix and mitigate against future regressions.
I recommend tracking down the root cause of the concurrent access in the STOMP implementation and adding protection there.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1037388502
Using `CopyOnWriteArrayList` is better than using `synchronized` relatively speaking, but I'm still not in favor of a change that is going to have such a widespread impact for an issue that seems confined to the STOMP implementation. As noted previously, I recommend you track down the root cause of the concurrent access and add protection there.
As far as testing goes. I will typically write a system/integration test that brute forces the issue to occur. For example, if there's a race condition in connection handling then I'll spin up a few hundred threads and create as many connections as possible as quickly as possible. Once I can brute force the issue semi-reliably then I'll gather logging to narrow down the issue until the root cause is clear at which point a proper fix can be 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1052454399
I'm closing this for now. Feel free to resubmit when you deal with the issues listed in the comments. Thanks!
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] BBE78 commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
BBE78 commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1037317911
@jbertram do you prefer my last commit (by the use of a CopyOnWriteArrayList)?
@jbertram and @clebertsuconic I didn't see how I could verify this modification (and also the initial problem) through a simple unit test. Any idea?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1036467831
FWIW, the build failure was spurious so no need to be concerned about that specifically.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1036505818
a test would be even nice to validate the synchronize is a the right place..
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram closed pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
jbertram closed pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] BBE78 commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
BBE78 commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1036415838
I don't really understand the build failed relationship with my modifications...
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] BBE78 commented on pull request #3952: Fix for ARTEMIS-3509
Posted by GitBox <gi...@apache.org>.
BBE78 commented on pull request #3952:
URL: https://github.com/apache/activemq-artemis/pull/3952#issuecomment-1037317911
@jbertram do you prefer my last commit (by the use of a CopyOnWriteArrayList)?
@jbertram and @clebertsuconic I didn't see how I could verify this modification (and also the initial problem) through a simple unit test. Any idea?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org