Fix usage of ProcessCanceledException to support 2024.2 #1047

vaisakhkannan · 2024-10-22T08:04:18Z

Part of #767

I have created a new common class called ExceptionUtil, which other quick-fix classes use to call the common exception methods via lambda expression, in order to avoid code duplication.

…lass

TrevCraw

A couple overview thoughts:

We should not be using System.out.println anywhere, we should be writing to a log file.
In the spots where System.out.println is being used (the "unsuccessful" cases), has it been considered how we should handle being "unsuccessful"? Is it OK to just log an error and move on? Is there other handling we need to do?
I still see patterns of code that are duplicated quite often, mainly this pattern here:

Boolean success = ExceptionUtil.executeWithExceptionHandling(
				() -> {
					WorkspaceEdit we = context.convertToWorkspaceEdit(proposal);
					toResolve.setEdit(we);
					return true;
				},
				e -> LOGGER.log(Level.WARNING, "Unable to create workspace edit for code action to insert missing annotation", e)
		);
		if (success == null || !success) {
			System.out.println("An error occurred during the code action resolution.");
		}

It should be possible to write a common method to cover this code pattern.

vaisakhkannan · 2024-10-28T06:45:03Z

...berty/tools/intellij/lsp4mp4ij/psi/core/java/validators/annotations/AnnotationValidator.java

@@ -93,7 +93,10 @@ public String validate(String value, AnnotationAttributeRule rule) {
 				return null;
 			}
 			return rule.validate(value);


For the code below, the usage of the try block is different, so I used standard exception handling for those cases.

This looks structurally similar to what you refactored into the utility method. I'm not sure why the pattern wouldn't also apply here.

Hi @mrglavas , I made the changes based on your suggestions

mrglavas · 2024-10-30T17:19:59Z

src/main/java/io/openliberty/tools/intellij/lsp4jakarta/lsp4ij/AnnotationUtil.java

+    private static <T> T executeWithExceptionHandling(Supplier<T> action, Supplier<T> fallback) {
        try {
-            // Construct a stream of only the annotations applied to the type that are also
-            // recognised annotations found in scopes.
-            return Arrays.stream(type.getAnnotations()).map(annotation -> annotation.getNameReferenceElement().getQualifiedName())
-                    .filter(scopes::contains).distinct().collect(Collectors.toList());
-        } catch (IndexNotReadyException | ProcessCanceledException | CancellationException e) {
+            return action.get();
+        } catch (ProcessCanceledException e) {
+            //Since 2024.2 ProcessCanceledException extends CancellationException so we can't use multi-catch to keep backward compatibility
+            throw e;
+        } catch (IndexNotReadyException | CancellationException e) {
            throw e;
        } catch (Exception e) {
-            return Collections.<String>emptyList();
+            return fallback.get(); // Return fallback value
        }
    }


I see this method is private and only called by getScopeAnnotations(). Is there some way to combine this with the more widely used utility method you introduced in ExceptionUtil? Otherwise this has just introduced more complexity without reducing code duplication in this spot.

Yes @mrglavas , I made the changes , please take a look , Thanks

mrglavas · 2024-10-30T18:00:33Z

A couple overview thoughts:

1. We should not be using `System.out.println` anywhere, we should be writing to a log file.

2. In the spots where `System.out.println` is being used (the "unsuccessful" cases), has it been considered how we should handle being "unsuccessful"? Is it OK to just log an error and move on? Is there other handling we need to do?

3. I still see patterns of code that are duplicated quite often, mainly this pattern here:

Boolean success = ExceptionUtil.executeWithExceptionHandling(
				() -> {
					WorkspaceEdit we = context.convertToWorkspaceEdit(proposal);
					toResolve.setEdit(we);
					return true;
				},
				e -> LOGGER.log(Level.WARNING, "Unable to create workspace edit for code action to insert missing annotation", e)
		);
		if (success == null || !success) {
			System.out.println("An error occurred during the code action resolution.");
		}

It should be possible to write a common method to cover this code pattern.

I believe there's been some iteration on this PR since @TrevCraw 's initial review, but I'm still seeing duplication of the exception handling pattern in this PR. In the multiple place where the utility method is being used, I think those look great but wanted to point out that many of the changes made are in LSP4MP4IJ which is code that we basically copy out of Quarkus Tools. I think it would be beneficial to contribute a PR to IntelliJ Quarkus (https://github.com/redhat-developer/intellij-quarkus) which introduces the utility method which we would also use in Liberty Tools. This would eliminate the cost of having to apply these changes every time we do an update for LSP4MP4IJ.

vaisakhkannan · 2024-11-01T12:11:15Z

A couple overview thoughts:
1. We should not be using `System.out.println` anywhere, we should be writing to a log file.

2. In the spots where `System.out.println` is being used (the "unsuccessful" cases), has it been considered how we should handle being "unsuccessful"? Is it OK to just log an error and move on? Is there other handling we need to do?

3. I still see patterns of code that are duplicated quite often, mainly this pattern here:
Boolean success = ExceptionUtil.executeWithExceptionHandling(
				() -> {
					WorkspaceEdit we = context.convertToWorkspaceEdit(proposal);
					toResolve.setEdit(we);
					return true;
				},
				e -> LOGGER.log(Level.WARNING, "Unable to create workspace edit for code action to insert missing annotation", e)
		);
		if (success == null || !success) {
			System.out.println("An error occurred during the code action resolution.");
		}
It should be possible to write a common method to cover this code pattern.
I believe there's been some iteration on this PR since @TrevCraw 's initial review, but I'm still seeing duplication of the exception handling pattern in this PR. In the multiple place where the utility method is being used, I think those look great but wanted to point out that many of the changes made are in LSP4MP4IJ which is code that we basically copy out of Quarkus Tools. I think it would be beneficial to contribute a PR to IntelliJ Quarkus (https://github.com/redhat-developer/intellij-quarkus) which introduces the utility method which we would also use in Liberty Tools. This would eliminate the cost of having to apply these changes every time we do an update for LSP4MP4IJ.

Hi @mrglavas I can open PR against repo https://github.com/redhat-developer/intellij-quarkus to update for LSP4MP4IJ

mrglavas · 2024-11-01T15:49:52Z

...berty/tools/intellij/lsp4mp4ij/psi/core/java/validators/annotations/AnnotationValidator.java

+					}
+					return rule.validate(value);
+				},
+				null,  // Fallback value in case of exception


I'm pretty sure this will generate a NullPointerException. Should be a supplier that returns null. I see other instances of this in the code.

@mrglavas , Thanks for the suggestions, Yes you are correct that will get a NullPointerException.
So I initially have a thought of changing code like below

public String validate(String value, AnnotationAttributeRule rule) { return ExceptionUtil.executeWithExceptionHandling( () -> { if (rule == null) { return null; } return rule.validate(value); }, () -> null, // Fallback value supplier in case of exception e -> LOGGER.log(Level.WARNING, e.getLocalizedMessage(), e) ); }

Then based on your below comment

NullPointerException? Based on the above code, the lambda expression should be re-throwing the exception. Seems like you need a function (with the exception as the parameter) for the fallback rather than a supplier.

I made change to the method executeWithExceptionHandling

public static <T> T executeWithExceptionHandling(Supplier<T> action, Function<Exception, T> fallback) { try { return action.get(); } catch (ProcessCanceledException e) { //Since 2024.2 ProcessCanceledException extends CancellationException so we can't use multi-catch to keep backward compatibility //TODO delete block when minimum required version is 2024.2 throw e; } catch (IndexNotReadyException | CancellationException e) { throw e; } catch (Exception e) { // Invoke the fallback function with the exception to generate a safe return value. return fallback.apply(e); } }

Please review the changes and provide any suggestions.

mrglavas · 2024-11-01T15:50:19Z

...y/tools/intellij/lsp4mp4ij/psi/internal/core/java/corrections/JavaDiagnosticsDefinition.java

+                getInstance().beginDiagnostics(context);
+                return true;
+            },
+                null,


NullPointerException? See my previous comment.

I made the change like this

@Override public void beginDiagnostics(JavaDiagnosticsContext context) { ExceptionUtil.executeWithExceptionHandling( () -> { getInstance().beginDiagnostics(context); return true; }, e -> { LOGGER.log(Level.WARNING, "Error while calling beginDiagnostics", e); return null; } ); }

This won't give NullPointerException.

mrglavas · 2024-11-01T15:50:44Z

...y/tools/intellij/lsp4mp4ij/psi/internal/core/java/corrections/JavaDiagnosticsDefinition.java

+                getInstance().endDiagnostics(context);
+                return true;
+            },
+                null,


NullPointerException? See my previous comment.

Did change similar to above comment

@Override public void endDiagnostics(JavaDiagnosticsContext context) { ExceptionUtil.executeWithExceptionHandling( () -> { getInstance().endDiagnostics(context); return true; }, e -> { LOGGER.log(Level.WARNING, "Error while calling endDiagnostics", e); return null; } ); }

mrglavas · 2024-11-01T15:53:50Z

...ellij/lsp4mp4ij/psi/internal/faulttolerance/java/MicroProfileFaultToleranceASTValidator.java

+				PsiType methodReturnType = node.getReturnType();
+				return methodReturnType.getCanonicalText();
+			},
+				null,


NullPointerException? Based on the above code, the lambda expression should be re-throwing the exception. Seems like you need a function (with the exception as the parameter) for the fallback rather than a supplier.

Thanks for the suggestion , I made the change in method executeWithExceptionHandling

please check my previous comment here

@vaisakhkannan It does not seem like you resolved this issue. It looks like the code is returning null instead of re-throwing the exception which is in the original code. If you're looking for ideas on how to handle this scenario, this post on Stack Overflow might be helpful: https://stackoverflow.com/questions/18198176/java-8-lambda-function-that-throws-exception

@mrglavas, Thanks for review comment and link you shared, I modified the code to re-throwing the exception, We can't use throw e, so I used,
throw new RuntimeException("Failed to validate asynchronous annotation", e);

By throwing a RuntimeException, it signals that something went wrong.

@vaisakhkannan The link I shared demonstrates exactly how you can re-throw the exception using a custom functional interface, so this is technically possible to do. The code you are modifying comes from IntelliJ Quarkus and without in-depth understanding of all the paths we could enter this method, just changing this to a generic RuntimeException could change the behaviour. A wrapped exception is not the same. The goal here is to improve how we do this exception handling without changing what it's doing. If the current usage can only produce unchecked exceptions, perhaps we should change the catch from Exception to RuntimeException and then the function could re-throw the RuntimeException without any restrictions.

mrglavas · 2024-11-01T15:54:26Z

src/main/java/io/openliberty/tools/intellij/util/ExceptionUtil.java

+                toResolve.setEdit(we);
+                return true;
+            },
+                null,


NullPointerException? See previous comments.

vaisakhkannan · 2024-11-05T06:57:11Z

I am attaching the recording of the failed test and logging the error message.

failed-test-new.mov

mrglavas · 2024-11-05T17:01:05Z

I am attaching the recording of the failed test and logging the error message.

@vaisakhkannan To be clear, I think what you're trying to show here is a demo of the fallback path and you've done that by forcing an exception to be thrown (with temporary code which divides by 0 that isn't in the PR). Thanks for showing that. I was concerned for a moment that this was about test failures in the build.

vaisakhkannan · 2024-11-06T03:38:48Z

done that by forcing an exception to be thrown (with temporary code which divides by 0 that isn't in the PR)

Yes @mrglavas , I tested reproducing exception by adding temporary code that forces an exception by dividing by zero, but that code isn't included in this PR.

mrglavas

See my most recent comment about support for re-throwing exceptions.

vaisakhkannan · 2024-11-07T12:38:06Z

src/main/java/io/openliberty/tools/intellij/util/ExceptionUtil.java

+            },
+            e -> {
+                logger.log(Level.WARNING, logMessage, e);
+                return false;


@mrglavas ,I replaced null with false , so it’s better to stick with false to clearly indicate failure without adding null ambiguity.

mrglavas

See my most recent comment about re-throwing exceptions.

src/main/java/io/openliberty/tools/intellij/util/ExceptionUtil.java

mrglavas

Looks good now. Thanks for addressing the review comments.

dessina-devasia

Looks good to me

…FIx-ProcessCanceledException Fix usage of ProcessCanceledException to support 2024.2

vaisakhkannan added 6 commits October 21, 2024 09:31

Fix Multi catch exception

e663b58

Updated header year

aa9d11d

Added common class for exception handling

54aae6c

Updated AnnotationUtil.java

973a051

Refactored code to use common exception class

c4e4425

Refactored quickfix code to use common exception class

b4fd07f

vaisakhkannan requested review from mrglavas and aparnamichael October 22, 2024 08:04

vaisakhkannan added 2 commits October 22, 2024 13:46

Updated warning message

59632c6

Added code for MicroProfileFaultToleranceASTValidator to use common c…

7d0d98b

…lass

TrevCraw reviewed Oct 25, 2024

View reviewed changes

vaisakhkannan added 2 commits October 27, 2024 14:11

done more refactoring

f6609f5

Added more refactored code

d642c67

vaisakhkannan commented Oct 28, 2024

View reviewed changes

vaisakhkannan requested review from TrevCraw and dessina-devasia October 28, 2024 06:47

mrglavas reviewed Oct 30, 2024

View reviewed changes

vaisakhkannan added 3 commits November 1, 2024 16:04

Refactored some code

de48784

Modified code in AnnotationUtil.java class

5568f54

Completed whole refactoring

b048a86

vaisakhkannan requested a review from mrglavas November 1, 2024 12:13

mrglavas reviewed Nov 1, 2024

View reviewed changes

vaisakhkannan added 2 commits November 5, 2024 08:51

Replaced null with () -> null for the fallback value

f7a720f

Did change on method executeWithExceptionHandling

283a777

vaisakhkannan added 2 commits November 5, 2024 10:53

I added more comments and corrected the indentation issue.

659c6c6

Added comment back

92388f0

vaisakhkannan requested a review from mrglavas November 5, 2024 06:57

mrglavas requested changes Nov 6, 2024

View reviewed changes

vaisakhkannan added 2 commits November 7, 2024 14:52

Replaced null

5aa8e99

Replaced null with false

3255846

vaisakhkannan commented Nov 7, 2024

View reviewed changes

vaisakhkannan requested a review from mrglavas November 7, 2024 12:48

mrglavas requested changes Nov 7, 2024

View reviewed changes

mrglavas reviewed Nov 7, 2024

View reviewed changes

src/main/java/io/openliberty/tools/intellij/util/ExceptionUtil.java Outdated Show resolved Hide resolved

Changed to RuntimeException

d5a1c00

vaisakhkannan requested a review from mrglavas November 7, 2024 15:34

back to re-throw exception

c93a8dd

mrglavas approved these changes Nov 7, 2024

View reviewed changes

dessina-devasia approved these changes Nov 8, 2024

View reviewed changes

vaisakhkannan merged commit 0178ba9 into OpenLiberty:intellij-2024.2-support Nov 8, 2024
1 of 3 checks passed

dessina-devasia pushed a commit to dessina-devasia/liberty-tools-intellij that referenced this pull request Nov 25, 2024

Merge pull request OpenLiberty#1047 from vaisakhkannan/OpenLiberty#767-…

29657e7

…FIx-ProcessCanceledException Fix usage of ProcessCanceledException to support 2024.2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix usage of ProcessCanceledException to support 2024.2 #1047

Fix usage of ProcessCanceledException to support 2024.2 #1047

vaisakhkannan commented Oct 22, 2024

TrevCraw left a comment

vaisakhkannan Oct 28, 2024

mrglavas Oct 30, 2024

vaisakhkannan Nov 1, 2024

mrglavas Oct 30, 2024

vaisakhkannan Nov 1, 2024

mrglavas commented Oct 30, 2024

vaisakhkannan commented Nov 1, 2024

mrglavas Nov 1, 2024

vaisakhkannan Nov 5, 2024 •

edited

Loading

mrglavas Nov 1, 2024

vaisakhkannan Nov 5, 2024

mrglavas Nov 1, 2024

vaisakhkannan Nov 5, 2024

mrglavas Nov 1, 2024

vaisakhkannan Nov 5, 2024

mrglavas Nov 6, 2024

vaisakhkannan Nov 7, 2024 •

edited

Loading

mrglavas Nov 7, 2024

mrglavas Nov 1, 2024

vaisakhkannan commented Nov 5, 2024

mrglavas commented Nov 5, 2024

vaisakhkannan commented Nov 6, 2024

mrglavas left a comment •

edited

Loading

vaisakhkannan Nov 7, 2024

mrglavas left a comment

mrglavas left a comment

dessina-devasia left a comment

Fix usage of ProcessCanceledException to support 2024.2 #1047

Fix usage of ProcessCanceledException to support 2024.2 #1047

Conversation

vaisakhkannan commented Oct 22, 2024

TrevCraw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrglavas commented Oct 30, 2024

vaisakhkannan commented Nov 1, 2024

Choose a reason for hiding this comment

vaisakhkannan Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaisakhkannan Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaisakhkannan commented Nov 5, 2024

mrglavas commented Nov 5, 2024

vaisakhkannan commented Nov 6, 2024

mrglavas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrglavas left a comment

Choose a reason for hiding this comment

mrglavas left a comment

Choose a reason for hiding this comment

dessina-devasia left a comment

Choose a reason for hiding this comment

vaisakhkannan Nov 5, 2024 •

edited

Loading

vaisakhkannan Nov 7, 2024 •

edited

Loading

mrglavas left a comment •

edited

Loading