tkobayas commented on PR #4032:
URL:
https://github.com/apache/incubator-kie-tools/pull/4032#issuecomment-5946136267
@drccrd Thank you very much for this feature!
Here are some review suggestions from Claude. I think they are reasonable
points.
---
1. ProjectEngine.java:127 — worker.join() has no timeout (CONFIRMED)
If the Drools engine hangs (infinite loop, deadlock from a pathological
rule pattern), worker.join() blocks the single-threaded build executor forever.
All subsequent saves queue but never run — no compile diagnostics until the
server is restarted.
Suggested fix: Add a timeout to join() and treat expiry as a build
failure. Interrupt the worker to avoid leaking the thread.
```
// Replace:
worker.join();
// With:
worker.join(TimeUnit.SECONDS.toMillis(120));
if (worker.isAlive()) {
worker.interrupt();
throw new RuntimeException("Drools engine build timed out after 30
seconds");
}
```
[Toshiya] I think "Drools engine hangs during the build" is a very edge
case. But it's a good practice to have a timeout.
---
2. CompileDiagnostics.java:464 — fileFor() suffix matching picks wrong file
on name collision (PLAUSIBLE)
When multiple files share the same name (e.g. module-a/Rules.drl and
module-b/Rules.drl), the engine may report an error on the bare path Rules.drl.
fileFor() iterates fileByKfsPath and key.endsWith("/" + p) returns the first
iteration-order match, which may be the wrong file.
Suggested fix: Count matches — if more than one key matches the suffix,
fall back to fallback instead of picking an arbitrary one.
```
Path matched = null;
for (Map.Entry<String, Path> entry : fileByKfsPath.entrySet()) {
String key = entry.getKey();
if (p.endsWith("/" + key) || key.endsWith("/" + p) || p.equals(key)) {
if (matched != null) {
return fallback; // ambiguous — don't guess
}
matched = entry.getValue();
}
}
return matched != null ? matched : fallback;
```
---
3. DroolsLspServer.java:185 — Empty classpath leaves stale engine with
closed classloader (PLAUSIBLE)
When rebuildClassIndex() receives an empty classpathEntries (e.g.
transient Maven resolution failure), it skips the engine replacement block. The
previous engine remains installed, but its URLClassLoader was already closed by
the prior setEngine() call. The next compile attempt throws from the closed
loader.
Suggested fix: When classpath is empty, explicitly clear the engine
rather than leaving the stale one in place.
```
if (!classpathEntries.isEmpty()) {
Set<Path> engineClasspath = new LinkedHashSet<>(classpathEntries);
engineClasspath.addAll(buildOutputDirs);
textService.compileDiagnostics().setEngine(ProjectEngine.over(engineClasspath));
} else {
textService.compileDiagnostics().setEngine(null);
}
```
---
4. BuildMessageRanges.java:96 — Token search hits comments/strings before
real code (PLAUSIBLE)
findTokenInWindow searches the original text rather than the
already-computed masked text. If a token appears in a comment (e.g. // fix totl
field later) before the actual misspelled identifier in a pattern, and the
engine reports line 0 (no line info), the diagnostic range points at the
comment instead of the code.
Suggested fix: Search masked instead of text for the token position.
masked is already computed at line 93 but only used for
isInsidePattern/endOfClause.
```
// Line 96: change text to masked
int hit = findTokenInWindow(masked, token, searchStart, searchEnd);
// Line 100: same change
hit = findTokenInWindow(masked, matched, searchStart, searchEnd);
```
---
5. DroolsLspServer.java:257 — addAll(buildOutputDirs) is a no-op (CONFIRMED)
In setResolvedClasspath(), buildOutputDirs is derived from
classpathEntries via filterDirectories() — it is a subset. Adding it back with
engineClasspath.addAll(buildOutputDirs) adds nothing. The code works correctly
but is redundant and misleading about intent (contrast with rebuildClassIndex()
where buildOutputDirs can contain directories not in classpathEntries).
Suggested fix: Remove the redundant addAll.
```
// Replace:
Set<Path> engineClasspath = new LinkedHashSet<>(classpathEntries);
engineClasspath.addAll(buildOutputDirs);
textService.compileDiagnostics().setEngine(ProjectEngine.over(engineClasspath));
// With:
textService.compileDiagnostics().setEngine(ProjectEngine.over(classpathEntries));
```
---
6. DroolsLspDocumentService.java:194 — openTextAt() is O(n) per call, called
per sibling per compile (CONFIRMED)
Each call iterates all entries in sourcesMap and normalizes every path
via toAbsolutePath().normalize(). For a workspace with 200 open DRL files and a
group of 50 siblings, one save triggers 50 × 200 = 10,000 path normalizations.
On network mounts, these syscalls add measurable latency to every compile cycle.
Suggested fix: Maintain a reverse lookup map (Path → String) that is
updated on didOpen/didClose/didChange, so openTextAt is O(1).
```
// Add field:
private final Map<Path, String> openTextByPath = new ConcurrentHashMap<>();
// Update in didOpen/didChange:
Path normalized = toPath(uri).toAbsolutePath().normalize();
openTextByPath.put(normalized, text);
// Update in didClose:
openTextByPath.remove(normalized);
// Replace openTextAt:
private String openTextAt(Path path) {
return openTextByPath.get(path);
}
```
--
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]
