dcapwell commented on code in PR #2104:
URL: https://github.com/apache/cassandra/pull/2104#discussion_r1130030758
##########
src/java/org/apache/cassandra/streaming/StreamSession.java:
##########
@@ -1338,18 +1339,26 @@ public String toString()
'}';
}
- public StringBuilder boundStacktrace(StackTraceElement[] stacktrace, int
limit)
+ public static void boundStackTrace(Throwable e, int limit,
HashSet<Throwable> visited)
Review Comment:
you have no return... so you are just walking throwable and mutating them
which is not at all safe.
##########
src/java/org/apache/cassandra/streaming/StreamSession.java:
##########
@@ -1338,18 +1339,26 @@ public String toString()
'}';
}
- public StringBuilder boundStacktrace(StackTraceElement[] stacktrace, int
limit)
+ public static void boundStackTrace(Throwable e, int limit,
HashSet<Throwable> visited)
{
- if (limit >= stacktrace.length) {
- throw new IllegalArgumentException("Invalid limit size: " + limit);
- }
+ if (e == null)
+ return;
+
+ if (visited.contains(e))
+ throw new IllegalArgumentException("Exception cycle detected");
+ else
+ visited.add(e);
- StringBuilder out = new StringBuilder(limit);
- for (int i = 0; i < limit; i++)
+ if (e.getStackTrace().length == 0)
{
- out.append('\n');
- out.append('\t').append(stacktrace[i]);
+ if (e != null)
+ boundStackTrace(e.getCause(), limit, visited);
+ return;
}
- return out;
+
+ StackTraceElement[] stackTrace = e.getStackTrace();
+ StackTraceElement[] limitedStackTrace = Arrays.copyOfRange(stackTrace,
0, limit);
+ e.setStackTrace(limitedStackTrace);
Review Comment:
you should not be changing the stack trace of exceptions in production code,
this can have impact you are not aware of; it is not safe to make things
mutable that were not planned to be mutable
##########
src/java/org/apache/cassandra/streaming/StreamSession.java:
##########
@@ -1338,18 +1339,26 @@ public String toString()
'}';
}
- public StringBuilder boundStacktrace(StackTraceElement[] stacktrace, int
limit)
+ public static void boundStackTrace(Throwable e, int limit,
HashSet<Throwable> visited)
{
- if (limit >= stacktrace.length) {
- throw new IllegalArgumentException("Invalid limit size: " + limit);
- }
+ if (e == null)
+ return;
+
+ if (visited.contains(e))
Review Comment:
your set isn't pointer identity, it is `Object.equals + Objects.hashCode`
based, which is not safe for what you are doing.
##########
src/java/org/apache/cassandra/streaming/StreamSession.java:
##########
@@ -1338,18 +1339,26 @@ public String toString()
'}';
}
- public StringBuilder boundStacktrace(StackTraceElement[] stacktrace, int
limit)
+ public static void boundStackTrace(Throwable e, int limit,
HashSet<Throwable> visited)
{
- if (limit >= stacktrace.length) {
- throw new IllegalArgumentException("Invalid limit size: " + limit);
- }
+ if (e == null)
+ return;
+
+ if (visited.contains(e))
+ throw new IllegalArgumentException("Exception cycle detected");
+ else
+ visited.add(e);
- StringBuilder out = new StringBuilder(limit);
- for (int i = 0; i < limit; i++)
+ if (e.getStackTrace().length == 0)
{
- out.append('\n');
- out.append('\t').append(stacktrace[i]);
+ if (e != null)
Review Comment:
you should touched `e` so this null check doesn't do anything
##########
src/java/org/apache/cassandra/streaming/StreamSession.java:
##########
@@ -1338,18 +1339,26 @@ public String toString()
'}';
}
- public StringBuilder boundStacktrace(StackTraceElement[] stacktrace, int
limit)
+ public static void boundStackTrace(Throwable e, int limit,
HashSet<Throwable> visited)
{
- if (limit >= stacktrace.length) {
- throw new IllegalArgumentException("Invalid limit size: " + limit);
- }
+ if (e == null)
+ return;
+
+ if (visited.contains(e))
+ throw new IllegalArgumentException("Exception cycle detected");
+ else
+ visited.add(e);
Review Comment:
nit: `add` returns boolean saying if it was added or not, so you can do `if
(!visited.add(e)) throw new IllegalArgumentException("Exception cycle
detected");`
##########
src/java/org/apache/cassandra/streaming/StreamSession.java:
##########
@@ -1338,18 +1339,26 @@ public String toString()
'}';
}
- public StringBuilder boundStacktrace(StackTraceElement[] stacktrace, int
limit)
+ public static void boundStackTrace(Throwable e, int limit,
HashSet<Throwable> visited)
{
- if (limit >= stacktrace.length) {
- throw new IllegalArgumentException("Invalid limit size: " + limit);
- }
+ if (e == null)
+ return;
+
+ if (visited.contains(e))
+ throw new IllegalArgumentException("Exception cycle detected");
+ else
Review Comment:
nit: don't need an else
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
