You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "pat-lego (via GitHub)" <gi...@apache.org> on 2023/06/28 14:30:23 UTC
[GitHub] [sling-org-apache-sling-event] pat-lego opened a new pull request, #31: Fix infinite recursion issue: SLING-11918
pat-lego opened a new pull request, #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31
Adding the ability to retry up to 10 times but no more.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-event] stefan-egli commented on a diff in pull request #31: Fix infinite recursion issue: SLING-11918
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31#discussion_r1250696403
##########
src/main/java/org/apache/sling/event/impl/jobs/stats/GaugeSupport.java:
##########
@@ -150,8 +154,10 @@ private void registerWithSuffix(String suffix, int count, Gauge<Long> value) {
metricRegistry.register(metricName, value);
gaugeMetricNames.add(metricName);
} catch (IllegalArgumentException e) {
- if (queueName != null) {
+ if (queueName != null && count <= 10) {
registerWithSuffix(suffix, count + 1, value);
+ } else {
+ logger.debug("Failed to register suffix {} for the queue {}, attempt {}", suffix, queueName, count, e);
Review Comment:
What about logging an error if it hits `10` and logging debug in any other case? eg like so:
```suggestion
if (queueName != null && count <= 10) {
logger.debug("Failed to register suffix {} for the queue {}, attempt {}, retrying.", suffix, queueName, count, e);
registerWithSuffix(suffix, count + 1, value);
} else {
logger.error("Failed to register suffix {} for the queue {}, attempt {}, giving up.", suffix, queueName, count, e);
```
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-event] klcodanr commented on pull request #31: Fix infinite recursion issue: SLING-11918
Posted by "klcodanr (via GitHub)" <gi...@apache.org>.
klcodanr commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31#issuecomment-1615004387
Re: the build failure, looks like it's Java 17: [SLING-11923](https://issues.apache.org/jira/browse/SLING-11923)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-event] sagarmiglani commented on a diff in pull request #31: Fix infinite recursion issue: SLING-11918
Posted by "sagarmiglani (via GitHub)" <gi...@apache.org>.
sagarmiglani commented on code in PR #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31#discussion_r1246307588
##########
src/main/java/org/apache/sling/event/impl/jobs/stats/GaugeSupport.java:
##########
@@ -150,8 +154,10 @@ private void registerWithSuffix(String suffix, int count, Gauge<Long> value) {
metricRegistry.register(metricName, value);
Review Comment:
In my opinion, if we are aware that the metricRegistry throws an `IllegalArgumentException (IAE)` when a name already exists, wouldn't it be better if we use gaugeMetricNames to skip the already existing names and avoid catching `IAE`?
Furthermore, instead of relying on a count, wouldn't it be better to utilize a randomly generated key to minimize the probability of collisions?
While I have limited knowledge about this module, I believe, by limiting the count, we are restricting the number of metrics with the same name which was not the case before.
In my view, using a random key would have prevented the potential situation of an infinite loop. WDYT?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-event] pat-lego commented on pull request #31: Fix infinite recursion issue: SLING-11918
Posted by "pat-lego (via GitHub)" <gi...@apache.org>.
pat-lego commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31#issuecomment-1629019607
@stefan-egli yes please merge the PR I cannot do it
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-event] stefan-egli merged pull request #31: Fix infinite recursion issue: SLING-11918
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli merged PR #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-event] klcodanr commented on pull request #31: Fix infinite recursion issue: SLING-11918
Posted by "klcodanr (via GitHub)" <gi...@apache.org>.
klcodanr commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31#issuecomment-1614988411
I could see how using a random string could be helpful, but it would also make it harder to correlate which metrics belong to the duplicated configuration since theoretically they should all be incrementing. TBH, unless there's a good reason to introduce the complexity, I'd keep the fix focused on the infinite recursion problem.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-event] pat-lego commented on pull request #31: Fix infinite recursion issue: SLING-11918
Posted by "pat-lego (via GitHub)" <gi...@apache.org>.
pat-lego commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31#issuecomment-1621855443
@stefan-egli included the recommendation into the branch, the build is failing due to the incompatibility of this project with Java 17 which is addressed by [SLING-11923](https://issues.apache.org/jira/browse/SLING-11923)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-event] pat-lego commented on pull request #31: Fix infinite recursion issue: SLING-11918
Posted by "pat-lego (via GitHub)" <gi...@apache.org>.
pat-lego commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/31#issuecomment-1613237257
@sagarmiglani logically you are correct but you are missing one critical piece of information. There are scenarios that cause events to already register this mbean and at times this code fires after this mbean has already been registered. We have just recently witnessed this.
This infinite loop causes a stackoverflow error and because of this can cause eventually cause a deadlock in the system because the necessary finally block is never called to release any of the necessary semaphores that are acquired higher in the call stack.
In my professional opinion, recursion with no way out (no base case) is a poor design and will lead to issues that we have just witnessed.
If we want to try and perform a different way to prevent a collision that is fine but the main issue here is that we have recursion that theoretically can never end. This is the critical issue because when you get stack overflow errors the function can never complete, finally blocks do not get called and resources never get released which cause system deadlocks.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org