Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: IllegalArgumentException in Colorizer event processing. #856

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

andrewL-avlq
Copy link
Contributor

@andrewL-avlq andrewL-avlq commented Jan 10, 2025

The Colorizer should not process stale tokens if they go over the end of the line in the current document. This prevents IllegalArgumentException occurring when the Tokenizer thread runs slow or is delayed relative to the textChanged notification in the reconciler.

@github-actions github-actions bot added the bug label Jan 10, 2025
@andrewL-avlq
Copy link
Contributor Author

I think this should fix issues like #408.

@sebthom sebthom self-assigned this Jan 10, 2025
@sebthom
Copy link
Member

sebthom commented Jan 10, 2025

Thanks for your PR. However I am not sure if this is the right fix or if we should better suppress the IllegalArgumentException as well as the BadLocationExceptions in the catch block and only log them when tracing is enabled. This new check does not fully prevent the occurrence of race conditions in the following lines. Also I am not sure if continuing to colorize subsequent lines makes sense if the tokens of one line are stale.

@andrewL-avlq
Copy link
Contributor Author

andrewL-avlq commented Jan 10, 2025

Sure, this PR is a starting point for discussion if nothing else.

The edits that the Tokenizer is processing might not include changes to the following lines, so exiting the colorizer early didn't seem quite right. On the other hand, when the Tokenizer thread has updated the tokens, the colorizer will run again, so I am fine with throwing if you want.

Would you rather catch the IllegalArgumentException, or detect the condition early (such as having getTokenLength detect that it is about to return a negative value) and throw a custom exception (maybe even BadLocationException)?

Given that the Tokenizer thread runs asynchronously, I was not trying to prevent any racing, but rather to have the colorizer cope with what goes wrong if the model is out of sync with the document (as there already appear to be conditions to handle other cases of this). Also for this reason, I agree that the logging of the BadLocationExceptions should be guarded and only logged when tracing is enabled.

@sebthom
Copy link
Member

sebthom commented Jan 10, 2025

I think I would just add another } catch (final IllegalArgumentException | BadLocationException ex) { error = ex; TMUIPlugin.logTrace(ex); } block with some explanation before

} catch (final Exception ex) {
error = ex;
TMUIPlugin.logError(ex);
} finally {

If the tokenizer thread runs slow or is delayed relative to the document
changed handling, then it is expected that BadLocationExceptions or
IllegalArgumentExceptions happen within the Colorizer. Exception
handling is now fixed to suppress the logging of these exceptions unless
tracing is enabled.
@andrewL-avlq andrewL-avlq force-pushed the ColorizerTokenProcessing branch from d457dd7 to 728aaa5 Compare January 13, 2025 10:56
@sebthom sebthom merged commit 69feea4 into eclipse-tm4e:main Jan 13, 2025
19 of 20 checks passed
@sebthom
Copy link
Member

sebthom commented Jan 13, 2025

Thanks!

@andrewL-avlq
Copy link
Contributor Author

No problem.
What is the timescale for seeing this in a release?

@sebthom
Copy link
Member

sebthom commented Jan 13, 2025

There is no fixed plan. I can create a release when the license check works again. I already opened a support ticket.

@andrewL-avlq
Copy link
Contributor Author

That would be great thanks.

@sebthom
Copy link
Member

sebthom commented Jan 14, 2025

@andrewL-avlq A new release 0.14.1 is available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants