LspSupport API + lib_runner for LSP/tool support

[lagergren]

A review vehicle, not a merge request. Opened on @cpurdy's branch so the design has somewhere to be commented on. @cpurdy / @ggleyzer: retarget, take it over, or close it as you like.

What this reviews

2568d6be4 — LspSupport (compile ×2, run ×2, a Control handle, the TC-xx diagnostic vocabulary), lib_runner as the Container-0 supervisor, and per-run consoles as named native resources.

I rewrote this description because the original one was written against 4fb0c8505 and had gone stale: it described ToolConnector.java (renamed since), called lib_runner a stub, and listed five // TODO GG sites. None of those remain — the branch works.

The findings below came from wiring my XtcEngine embedding to this branch and running existing tests through it, not from reading.

One worth answering before this lands

A standard XDK module cannot run under the runner. runner.x:161 uses BasicResourceProvider, which has no case for Directory or FileStore, so TestFiles (manualTests/src/main/x/files.x, in testModuleNames at manualTests/build.gradle.kts:467) fails with Invalid resource: Key: storage, FileStore. Switching to PassThroughResourceProvider fixes availability but gives up per-run isolation, since every run then resolves to container zero's instances. So neither stock provider is right, and the underlying gap is that runTask(template, repository, consoleId) has no way to say "this run gets its own resources" — the same gap as the UnsupportedOperationException at LspSupport.java:475. Detail in the diff.

Two things here I'd keep

The ErrorListener threaded through every compile/run entry point, and the TC-xx code vocabulary with documented %1/%2 params. A long-running host already owns a diagnostic sink, wants messages as they're produced and correlated with its own request, and wants something to switch on other than message text.

Everything else is inline on the diff, and most of it is small.

[lagergren]

Field-by-field notes, anchored to the code. The short version: all six non-final fields are written exactly once, so the class has no genuinely re-assignable state - and the three that are read without the lock are exactly the three that are logically write-once. Making them final/immutable is the same fix as the thread-safety gap, not a separate style preference.

Happy to push a commit for any of this, or leave it entirely to you - it's your branch.

[lagergren]

This field and the two below are written exactly once, inside configure(), which explicitly refuses to change them afterwards ("configuration has been performed, and cannot be modified"). So they are write-once by construction, but not declared that way.

They are also the fields read WITHOUT holding LOCK (see isConfigured() and getConfiguredRepository()), while the class javadoc promises "thread-safe and concurrent". Those two facts are the same problem: mutable-but-logically-final state published without a happens-before edge.

Collapsing the three into one immutable value fixes both at once:

private record Config(ModuleRepository repo, String injector) {}
private volatile Config config;   // null until configured; ONE atomic publication

configured then becomes derived (config != null) rather than stored, so the two can never disagree, and the unsynchronized reads become safe.

[lagergren]

Coming back to this with the other half, and with a correction to my own comment above.

The correction. I named isConfigured() and getConfiguredRepository() as the unsynchronized readers. It's eight sites, not two: :93, :103, :170, :177, :185, :211, :459, :474. Only configure (:152) and ensureConnector (:122) take LOCK.

The record Config half stands as written: one immutable value behind one volatile, so the publication is atomic and "half configured" stops being representable — the flag and the values can't disagree because there is no flag. configure then compares whole values, which is what its error message already claims to do.

The half I hadn't worked out in August is connector, and it reverses what I said on the sibling thread about it having a genuine reason not to be final. It doesn't. As private final Lazy.Bound<LspSupport, Connector> it's computed at most once and published with volatile ordering by the holder itself (Lazy.java:432-457, double-checked through a VarHandle), so ensureConnector needs no lock. And once neither configure's readers nor ensureConnector need it, the static LOCK can be deleted rather than worked around — which is the part worth having, since it's a static lock guarding instance state, correct only while exactly one instance exists.

Lazy isn't a new dependency: AbstractConverterMap already uses Lazy.ofBound for three fields (javatools_utils/src/main/java/org/xvm/util/converter/AbstractConverterMap.java:30-43).

One deliberate behaviour change. createConnector now states its precondition. ensureConnector is public and never called verifyConfigured, so calling it before configure built a Connector over a null cfgRepo and failed later, somewhere else. Latent rather than reached today — LspTest.main configures at :49 before reaching ensureConnector at :184 — but it's reachable through a public method.

Written up as 618e1f9 on lagergren/lspapi-config-lifecycle — its own branch rather than with the small fixes, because it's a design decision you should be able to take or reject as a unit. :javatools:compileJava is clean. No test, for the reason in the LspTest thread.

[lagergren]

configured is read here without holding LOCK, and it is a non-volatile field written by configure() on another thread. There is no happens-before edge, so this can return a stale false (or, worse, true while cfgRepo is not yet visible).

Same for getConfiguredRepository() below, and for verifyConfigured(). The volatile Config shape suggested on the field declaration makes all three correct without taking the lock.

[lagergren]

BasicResourceProvider doesn't supply a file system, so a run that touches one dies here.

Its getResource switch handles Console, Clock, Timer, Random/rnd, String, List<String>, and enum/Destringable values built from string injections. There's no case for Directory or FileStore, so curDir, rootDir, homeDir and storage fall through to default: and come back as a deferred exception. (The HashCollector and Linker imports at the top of that method have no matching cases.)

Wiring this up and running TestFiles fails with Invalid resource: Key: storage, FileStore. That isn't a module I wrote for the occasion — manualTests/src/main/x/files.x injects @Inject("storage") FileStore at :5 and @Inject Directory curDir at :68, and it's listed in testModuleNames at manualTests/build.gradle.kts:467, in neither exclusion set. Any module that touches the file system is excluded from the runner as written, which is a large fraction of anything real.

The runner it replaces made the other choice: manualTests/src/main/x/runner.x:100 declares service RunnerResourceProvider(Console console) extends PassThroughResourceProvider. That's the same shape as TaskResourceProvider here — a console-supplying subclass — with a different base, and the base is the entire difference.

Pass-through isn't the fix either, though. I tried it: the missing resources come back, and two ownership tests then fail the other way round, because pass-through delegates to the parent, so every run resolves to container zero's instances instead of its own. Availability or isolation: the two stock providers sit at opposite ends, and a host needs both.

So the gap is in the signature rather than in the choice of provider. runTask(template, repository, consoleId) has nowhere for a caller to say "this run gets its own file system, rooted here" — no injector, no rootDir. It's the same gap seen from the Java side at LspSupport.java:475, where rootDir and a custom injector throw UnsupportedOperationException.

Worth closing for sequential runs on its own merits, and it happens to be one of the things concurrent runs would need anyway. Happy to take it as a separate branch if you'd like — it's an API change, so it wants to be accepted or rejected as a unit rather than patched inline.

[lagergren]

Or do I have to inject these kinds if things explicitly, but only in this configuration?

[ggleyzer]

You are 100% correct; this is on my list of TODOs. The plan to to create a dedicated "root" directory for every newly created container - very similar to the platform's HostInjector

[lagergren]

Confirmed, very nice and better than I expected: DirectoryFileStore(taskDir) with storage, rootDir, homeDir, curDir and tmpDir all answered from it is exactly the shape this thread said was missing: fabricate what has to be per-run, delegate the rest.

I should correct myself on one point. I had concluded the platform had no FileStore that could be rooted below / — I read the native xOSFileStore with its static final File ROOT = new File("/") and stopped there, without looking in lib_ecstasy/fs, where DirectoryFileStore has been all along, documented for precisely this ("the Container will not be able to see 'above' the level of the injected FileStore's 'root' directory"). You used the right tool and I was wrong about it not existing.

I've taken your runner into my embedding branch as-is and TestFiles runs under it.

One piece of this thread is still open, and it's the injections rather than the file system: TaskResourceProvider extends BasicResourceProvider, whose String case forwards to the parent, so two runs asking for the same name both resolve against container zero and see one value. LspSupport.run still throws for injections too. I have that working in my calling branch on my side as a pair of parallel String[] arrays on registerTask, with a test that runs one module twice with different values — happy to put it up separately if you want it.

[lagergren]

These three are outputs: Task writes them, and everything else — TaskRegistry, and the Java side through it — only reads. As declared they're writable by anything holding the service, so an outcome can be reported that never happened.

public/private says the intent and enforces it, and it's the idiom already used throughout lib_ecstasy (47 occurrences, e.g. HasherMap.x:108, AsyncSection.x:54):

public/private Boolean running;

public/private Int? result;

public/private String? failure;

status just below already has this shape — a computed getter with no setter at all — so this makes the other three consistent with it.

Fix: b561e3b on lagergren/lspapi-review-fixes.

[ggleyzer]

I'm changing all this rn

[lagergren]

This one didn't make it into the rework — Boolean running; on Task is still writable by anything holding the service, though everything around it changed.

public/private Boolean running; is the whole fix, and status just below already has that shape by being a computed getter.

[lagergren]

The holder idiom's guarantee is that a reader either sees a fully constructed instance or doesn't see the class initialized at all — and that rests on the field being static final. Without final, the JVM isn't required to give a reader that ordering, so the one property this pattern exists to provide isn't actually in force.

static final LspSupport INSTANCE = new LspSupport();

And the constructor on :60 is package-private with exactly one caller — this holder. private states that and keeps it true; nothing extends the class.

Also then the naming should be UPPER_SNAKE_CASE.

Fix: 6b92b36 on lagergren/lspapi-review-fixes.

This is the narrow finality point only — not the singleton itself, which I don't think should change: xExternalConsole.register reads a mutable static INSTANCE, and there are 144 templates shaped that way, so the runtime genuinely can't host two connectors today. The singleton here is a symptom of that, not the cause.

[lagergren]

Worth recording what this override buys, because it's the kind of thing a later cleanup undoes by accident.

Writing to hConsole.f_out here means a redirected run never touches CONSOLE_LOG. The parent's PRINT/PRINTLN continuations (xTerminalConsole.java:239-252) call CONSOLE_LOG.log(ach, …) on every print, and CONSOLE_LOG is a public static final ConsoleLog — one instance for the whole JVM — backed by:

private final String[] m_asLine = new String[1024];
private int            m_cLines = 0;
private int            m_iLine  = 0;

with zero occurrences of synchronized, volatile, Atomic or any lock in the entire file (javatools_utils/src/main/java/org/xvm/util/ConsoleLog.java). Two threads printing concurrently interleave read-modify-write on those cursors: lost lines, one line's text stored under another's index, a torn buffer in get/render. Nothing fails loudly, so the scrollback is just silently wrong.

That's a master defect, not this PR's — neither file is touched by this branch, and I'm raising it separately. Two things about it are this PR's business though:

1. This override is the mitigation. Anyone later "simplifying" it to delegate to super reintroduces the exposure for every hosted run at once. A line of comment here would stop that.

2. The fallback doesn't have it. A run with no console id gets BasicResourceProvider's case (Console, "console"), which resolves to the parent's terminal console — so it prints through CONSOLE_LOG after all. That's the path runTask(…, Null) takes today, which is every run the Java side currently starts. So concurrent runs without console ids still interleave on the unsynchronized buffer.

Fixing ConsoleLog itself is one class and no API change — synchronize log/size/get/render, or swap the ring buffer for a concurrent structure. Worth doing before a model that runs modules concurrently lands, rather than after.

[ggleyzer]

Yes, "a redirected run never touches CONSOLE_LOG". By design.

My assumption was that the concurrency responsibility lies on the provided PrintStream. If you think it's incorrect or too tolling for the user I'd be willing to reconsider

[lagergren]

A PrintStream is a mechanism that has no way of reporting an error, and it does things with flush behind the scenes for newlines and stuff. I don't think it is a great vehicle. If we do have consoles, or anything it is integrated with that we can reuse for that output, it's probably a good idea. Same in the debugger console that basically parses emitted stdout (if I understand it correctly). I can get by with the PrintStream here but I think it is a very limiting design choice - the main use case is probably a user invisible output status - log - but there can also be failures and so on. Leave it for now but I forsee very much that I will have to change it to something else later.

[lagergren]

A suggestion about the shape of the new API rather than about this line, which is correct as written.

MainContainer.invokeAsync returns CompletableFuture<ObjectHandle> (MainContainer.java:250), so every caller has to re-establish the type by hand. Three sites here, and all three are right:

taskId = ((JavaLong) hTaskId).getValue();                       // :96
        : ((JavaLong) result).getValue());                      // :168
        : ((StringHandle) result).getStringValue());            // :175

What's slightly awkward is that the type is known on both sides and dropped only in between. runner.x declares Int runTask(...), Int? taskResult(...), String? taskFailure(...); this file knows exactly what it expects. The boundary is the one place that knowledge doesn't exist, so it gets re-asserted with a cast the compiler can't check — and if a signature in runner.x changes later, the failure is a ClassCastException here rather than a compile error there.

I want to be clear this isn't an just LSP specific thing to fix: it's how the whole runtime is written, and there are ~1,200 casts to a *Handle type across javatools (grep -rE '\(\s*[A-Za-z_][A-Za-z0-9_.]*Handle\s*\)'). Nobody is going to unpick that here, and this PR adds three correct ones to it. We could at least stop it from growing with invokeAsync...

Something like:

<T extends ObjectHandle> CompletableFuture<T> invokeAsync(String name, Class<T> type, ObjectHandle... args)

and/or a couple of small helpers that unwrap Int and String? once, would put the knowledge in one place and mean the count stops growing at exactly the point where new code is being written. Much cheaper now, while the surface is this small, than once there are twenty call sites.

So not API issues per se, but somewhat offensive to a person working hard to on the Java Language Team building a strongly typed language that isn't used...

[ggleyzer]

MainContainer.invokeAsync() was introduced in this project. There are just three call sites, so we don't save much. But I don't disagree in prinicipal

[lagergren]

Exactly what I said - since it is new, let's type it properly so the Object downcasts don't grow at least. This is exactly how I would expect it to look and I would like that shape to incrementally be applied to other invokes and similar places. It also provides abstraction should we want to change something in e.g. ObjectHandle and it would turn runtime class cast exceptions today into something that simply won't compile, if we miss a case somewhere.

[lagergren]

The scenarios in here are the right ones — compile success and failure, run output, an exception propagating out of a run, latency, pool growth. My only concern is that nothing re-runs them.

The cheapest useful version is nearly free: annotate the five scenarios @Test. They already assert by throwing IllegalStateException, and a throwing test fails, so that alone buys CI coverage without rewriting a single check.

The one thing genuinely tied to main is the two path arguments:

dirLib       = new File(asArg[0]);
dirJavatools = new File(asArg[1]);

and there's an established answer to that a few files away: DirRepositoryConcurrentScanTest.java:44-50 resolves xdk/build/install/xdk/lib with a ../ fallback and then assumeTrue(...), so it skips cleanly when the XDK hasn't been built rather than failing noisily.

One scenario needs an actual assertion rather than just the annotation. testPoolGrows computes nativePoolSize(...) on each of its 13 iterations (i <= 12) and only prints it. Whatever the intended invariant is — that the pool stops growing once shapes repeat, or that each novel shape costs a bounded number of constants — a printed number can't check it, and it's the one measurement in this file that nothing looks at.

Not urgent for the design conversation, but it's what would make the rest of this checkable rather than a matter of opinion — mine included.

[ggleyzer]

Good point. I'm a bit hesitant to include long running tests in a CI run, but would take your advice on this one. You may have noticed - this is the reproducer you originally wrote)

[lagergren]

It doesn't have to be all-or-nothing, and the cheap version is nearly free.

The five scenarios already assert by throwing IllegalStateException, and a throwing test fails, so annotating them @Test alone buys CI coverage without rewriting a single check. If testPoolGrows and testRunLatency are the ones you're worried about, tag just those, or gate them on a property, and let the three fast ones run every commit.

The only thing genuinely tied to main is the two path arguments, and there's an in-tree pattern for that a few files away: DirRepositoryConcurrentScanTest.java:44-50 resolves xdk/build/install/xdk/lib with a ../ fallback and then assumeTrue(...), so it skips cleanly when the XDK hasn't been built rather than failing.

testPoolGrows is the one that needs an actual assertion rather than just the annotation — it computes nativePoolSize(...) on each iteration and only prints it, so whatever the intended invariant is, nothing checks it.

(And yes — noted that it started as my reproducer. All the more reason it would be nice to keep it running.)

Test caching also works well, locally, and can be added to the CI. I hope that I can use the embedding api to substantially also improve plugin performance and JVM forking time which is now one of the performance issues in the tests for cold runs with this.

[lagergren]

Construct-then-start is why three fields here can't be final — the object exists before the values do:

private volatile Instant started;     // write-once
private          long    taskId;      // write-once
private          Long    consoleId;   // write-once, then nulled to stay idempotent

Moving start()'s work into the constructor would be the wrong repair: a constructor that boots things fails at construction time for reasons unrelated to what the caller asked for. But the right place already exists — create is the factory, and it's the only caller. Give it the work, and hand the constructor finished values.

Then the class has no "constructed but not yet valid" state at all, seven fields are final, and only running/stopped/result stay mutable, which they honestly are: they're the outcome.

Two things fell out of doing it that I didn't expect:

• repository stops being a field. It's only read while starting — prepareModule and the xCoreRepository handle — so it becomes a parameter of a now-static prepareModule rather than state the control carries for its whole life.
• unregisterConsole no longer needs to null consoleId. That null-write exists to make it idempotent, because it's reachable from both start's catch and finish. Once the failure path lives in the factory — before any object exists — the only caller is finish, which is synchronized and returns early once running is false, so it happens exactly once by construction.

There's also a smaller point about taskId: today its safe publication rides on the watch() submission happening after the write. That's correct, but by accident of ordering rather than by design. final makes it correct by construction.

980d1a5 on lagergren/lspapi-control-finality, +43/−38 in this one file, :javatools:compileJava clean. Separate branch from the config one so they can be taken independently.

[ggleyzer]

let's revisit this one after my current change is in place

[ggleyzer]

please re-review

[lagergren]

Re-reviewed. The surrounding code improved a lot — completion replacing the poll, the failure path simplified — but this specific point is unchanged: it's still new InterpreterControl(interpreter, ...).start(), so started, taskId and consoleId still can't be final, and the object still exists before its values do.

Not urgent, and I'm not asking you to move the work into the constructor — that would be the wrong fix. The factory is already there and is already the only caller, so it can do the work and hand the constructor finished values. When I did that on our side, two things fell out that I hadn't predicted: repository stopped being a field at all (it's only read while starting), and unregisterConsole no longer needed to null consoleId to stay idempotent, because with the failure path in the factory its only caller is finish, which is synchronized and returns early once running is false.

[lagergren]

Concretely, since prose about a refactor is easy to disagree with. Applied to your current head (96a1473), compiles:

-        return new InterpreterControl(interpreter, module, repository, console, errs).start();
-    }
+        Instant started   = Instant.now();
+        Long    consoleId = console == null
+                ? null
+                : xExternalConsole.register(interpreter.getNativeContainer(), console);
+        try {
+            FileStructure file = prepareModule(interpreter, module, repository);
+            MainContainer main = interpreter.getMainContainer();
+            ...
+            ObjectHandle hTaskId = main.invokeAsync(
+                    "registerTask", hModule, hRepository, hConsoleId).join();
+            long taskId = ((JavaLong) hTaskId).getValue();
+
+            // the future exists before the task does, so the control can hold it as final
+            var completion = new CompletableFuture<ObjectHandle>();
+
+            InterpreterControl control = new InterpreterControl(interpreter, module, console, errs,
+                    started, consoleId, taskId, completion);
+
+            CLEANER.register(control, new TaskCleanup(interpreter, module.getSimpleName(), taskId));
+
+            main.invokeAsync("startTask", hTaskId).whenComplete((r, e) -> {
+                if (e == null) {
+                    ...
+                    control.finish(result, failure);
+                    completion.complete(r);
+                } else {
+                    control.finish(-1, e.toString());
+                    completion.completeExceptionally(e);
+                }
+            });
+            return control;
+        } catch (RuntimeException e) {
+            if (consoleId != null) {
+                xExternalConsole.unregister(interpreter.getNativeContainer(), consoleId);
+            }
+            throw e;
+        }
+    }

and the fields become:

    private final InterpreterConnector connector;
    private final ModuleStructure      module;
    private final PrintStream          console;
    private final ErrorListener        errs;
    private final Instant              started;
    private final Long                 consoleId;
    private final long                 taskId;
    private final CompletableFuture<ObjectHandle> completion;

    private volatile boolean running = true;
    private volatile Instant stopped;
    private volatile Long    result;

Three things worth pointing at:

• completion ends up final too, which I couldn't manage when I described this in prose. Create the empty future before constructing the control and complete it from the startTask continuation, instead of assigning a future onto the object afterwards.
• repository stops being a field. It's only read while starting, so it becomes a parameter of a now-static prepareModule rather than state the control carries for its whole life.
• unregisterConsole no longer nulls consoleId. That write only existed to make it idempotent across two callers; with the failure path in the factory, the only caller is finish, which is synchronized and returns early once running is false — idempotent by construction.

I deliberately left the CLEANER.register call exactly where it is. That's the separate conversation and I didn't want to bundle it in here.

Whole thing is d18f351 on lagergren/lspapi-h12-on-new-head if it's easier to read as a commit — it's 54 insertions, 42 deletions, one file.

[ggleyzer]

Implemented the suggested refactoring; it allowed to remove no longer necessary synchronization

[ggleyzer]

I think it's ready for a formal review now; un-drafting it

[lagergren]

Looks good. I have some comments - most importantly please use PrintWriter as suggested instead of the problematic Java 1.0 PrintStream with its exception swallowing and buffer issues, and I may have to extend further depending on what I discover I need to do + some pushback on the cleaner.

Furthermore the InterpreterControl init and my with for some more finality for the fields and not unnecessarily introducing and mutable state. The I have left open remaining simplifications and best practice comments that haven't been addressed but which would be nice, but are not life or death important.

[lagergren]

I would call it "EmbeddingSupport" instead of "LspSupport". LspSupport is a very specific use case for everything that can be done with this.

[lagergren]

One thing about close() itself, separate from the Cleaner question that thread settled.

if (running) {
    postRequest("killTask", xInt64.makeHandle(taskId)).join();
}
completion.join();
postRequest("deleteTaskDirectory", ...).join();

completion.join() is unbounded. The fourth concern I raised on the Cleaner — a join() that never returns — hasn't gone away; it has moved from a daemon thread nobody was watching to the caller's thread, which is a real improvement (it is visible, and it is attributable) but not a fix. A task that does not stop when killed now hangs whoever called close(), and in a try-with-resources that is the request thread of the host.

killTask first makes the common case fine. It is the uncommon one that worries me: a task wedged in a native call, or one the runtime cannot interrupt, leaves close() with nothing to time out against. And because closed is set before the joins, a second close() returns immediately rather than retrying — correct for idempotency, but it means there is no second chance either.

Two options, in preference order:

1. Bound the wait. completion.get(timeout, unit), and on timeout still issue deleteTaskDirectory and return. The root is what close() is really for; waiting for completion is a means to it, so a task that will not die should not also cost the caller its directory.
2. Do not wait at all when a kill was issued. If killTask has been sent, the delete can be chained off completion asynchronously rather than joined — close() returns once the intent is recorded.

Either way the caller gets a close() that terminates. Happy to be told this is theoretical if killTask is guaranteed to complete the task's future in bounded time — that is the part I cannot tell from here, and if it is guaranteed then a comment saying so on completion.join() would be enough.

[ggleyzer]

This was a valid concern with the previous kill() behavior. I changed the runtime semantics so that Container.kill() now forcefully terminates every service in the nested container and completes exceptionally all existing fibers, including fibers waiting on futures.

As a result, killTask cannot return while any task fiber remains alive. Once postRequest("killTask", ...).join() returns, the subsequent completion.join() is only waiting for the runner-side completion callback to publish the result and unregister the task; it no longer depends on the operations in the task itself.

[ggleyzer]

Also, renamed LspSupport to EmbeddingSupport

[ggleyzer]

I think I implemented all the suggestions; please re-review