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

Don't cache Java executors #1465

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Don't cache Java executors #1465

merged 2 commits into from
Nov 18, 2024

Conversation

lunaris
Copy link
Contributor

@lunaris lunaris commented Nov 18, 2024

Presently, the Java language host caches a single executor, which abstracts e.g. the command-line executions needed to run Java code. This is problematic should a single language runtime ever be fielded with concurrent requests for program execution. This commit removes this caching for now to fix this.

When we fail to execute a Java command, we currently duplicate the command name
(e.g. `mvn`), which makes it harder to understand what has gone wrong. This
commit cleans this up, additionally quoting the command we attempted to run so
that it's easier to parse in the event it contains spaces.
Presently, the Java language host caches a single executor, which abstracts e.g.
the command-line executions needed to run Java code. This is problematic should
a single language runtime ever be fielded with concurrent requests for program
execution. This commit removes this caching for now to fix this.
@lunaris lunaris added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Nov 18, 2024
@lunaris lunaris requested a review from a team as a code owner November 18, 2024 10:09
Copy link
Contributor

@tgummerer tgummerer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Creating these executors doesn't seem super expensive (We're looking for the paths for each of them on the filesystem in the worst case), so I don't think this is going to be a big detriment to performance.

Base automatically changed from wjones/better-exec-errors to main November 18, 2024 11:10
@lunaris lunaris merged commit 1b7a4cc into main Nov 18, 2024
23 checks passed
@lunaris lunaris deleted the wjones/exec-cache branch November 18, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants