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 kpsewhich timeout on Windows using Tex Live Full #3727

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

tristankretzer
Copy link
Contributor

@tristankretzer tristankretzer commented Nov 5, 2024

Fix #3653

Summary of additions and changes

  • Add a non-blocking command runner CommandRunnerKt.runCommandNonBlocking() and refactor SystemEnvironmentKt.runCommand() and SystemEnvironmentKt.runCommandWithExitCode() to use it as their base. The main differences are:
    • Input pipe is closed, if no input is provided.
    • If the output is not discarded, both output streams are read asynchronously. If the called program times out, the streams are still read until the pipe closes even if they are not already ready. This should not lead to problems as long as Java is able to kill the process. One could try to implement the readers in a less blocking way to at least make them abortable before something is read but this does seem to be a pretty complex problem.
    • The function is less blocking.
  • [Breaking] Change parameter nonBlocking of (former) SystemEnvironmentKt.runCommandWithExitCode() to discardOutput. The intent of users of this parameter seems to be to just don't care about the commands output, so this change seems to be safe. Also the implementation of the former behavior is not easy with the added asynchronous contexts for reading the streams.
  • Increase timeout of kpsewhich path expansion to 10 seconds to allow it to return and added logging of number of paths loaded. (I did not add a check only to increase the timeout on Windows, as is does not seem to block the IDE.)
  • [Edit 1] Add extension function String.runCommandNonBlocking() and replace usages of String.runCommand() in suspending functions.

I'm not quite sure if it runs in the background. The caller nl.hannahsten.texifyidea.startup.LatexPackageLocationCacheInitializer#execute() seems to run it in the background but I don't know how nl.hannahsten.texifyidea.util.files.LatexPackageLocationCache#getPackageLocation() is called. The former I could not provoke a hanging UI with, even with added delays. The latter I could not provoke a call to without having the cache filled first, so I could not check if it runs in background.

How to test this pull request

Verify the following document does not show error File 'article.cls' not found:

\documentclass{article}

\begin{document}
\end{document}
  • Updated the documentation, or no update necessary
  • Added tests, or no tests necessary

@tristankretzer
Copy link
Contributor Author

Setting this to draft to add a fix for Process.waitFor() blocking the full timeout instead of the command execution time.

@tristankretzer tristankretzer marked this pull request as draft November 6, 2024 20:01
@tristankretzer tristankretzer force-pushed the issue/3653 branch 4 times, most recently from 6331b8d to e86319f Compare November 9, 2024 19:47
@tristankretzer tristankretzer marked this pull request as ready for review November 9, 2024 20:25
Copy link
Collaborator

@PHPirates PHPirates left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the refactor! This is a great improvement I think.

Change parameter nonBlocking of (former) SystemEnvironmentKt.runCommandWithExitCode() to discardOutput.

Yeah that's fine, I think the parameter was just there to prevent issues with things getting stuck waiting for output that was not necessary

src/nl/hannahsten/texifyidea/util/CommandRunner.kt Outdated Show resolved Hide resolved
src/nl/hannahsten/texifyidea/util/CommandRunner.kt Outdated Show resolved Hide resolved
* @param returnExceptionMessageAsErrorOutput Whether to return exception messages as error output if exceptions are thrown.
* @param timeout The timeout for execution. Does not stop reading the process' output as long as it is available.
*/
suspend fun runCommandNonBlocking(
Copy link
Collaborator

Choose a reason for hiding this comment

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

but I don't know how nl.hannahsten.texifyidea.util.files.LatexPackageLocationCache#getPackageLocation() is called.

If I remove LatexPackageLocationCacheInitializer then it doesn't appear to be blocking the UI, at least it's running in a background thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea on how to test this.

Sadly I was now able to provoke a freezed UI. Steps to reproduce:

  1. Disable LatexPackageLocationCacheInitializer.
  2. Add to document following content
\documentclass{report}

\begin{document}
\end{document}
  1. Edit the file.

To make the effect more obvious, replace runBlocking { fillCacheWithKpsewhich(project) } with runBlocking { delay(1_000) }.

I can try to move the execution in another thread, but if this works depends on where the freezing occurs. Maybe another option is just to remove this call to fillCacheWithKpsewhich() as the cache should be filled at startup and getPackageLocation() is called multiple times - at least if it returns null. In either case I probably won't be able to evaluate this before next weekend.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, that does block the UI. But I don't understand, why is the UI not blocked if I put there runBlocking { runCommandNonBlocking("sleep", "3") } to simulate a long-running command?

I'm also not quite sure why the UI is blocked if we run runBlocking { delay(1_000) } in a background thread. Documentation: https://plugins.jetbrains.com/docs/intellij/threading-model.html#avoiding-ui-freezes

It's true that for this particular case we can maybe circumvent the issue, but there are other places where we are running system calls and need to wait for the result to return it to IntelliJ api, and I don't know how to do that without blocking the thread (and thus the UI, apparently?). Another example is LatexDocumentationProvider, which can be triggered in the autocompletion popup. (If I put a delay in runCommandWithExitCode then UI will block)

In any case, that is out of scope for this PR, so I'll merge this already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you make sure the command is executed? I had to circumvent the if-clause to test this:

if (true || cache == null) {
    runBlocking {
        runCommandNonBlocking("ping", "-n", "30", "127.0.0.1", discardOutput = true)
    }
}

With this I was able to block the UI until timeout.

As to why it blocks the UI, I'm not really sure as the debugger implies that those are indeed run in separate threads.

src/nl/hannahsten/texifyidea/util/CommandRunner.kt Outdated Show resolved Hide resolved
Add function `runCommandNonBlocking()` to run shell commands in a
non-blocking way and refactor `runCommand()` and
`runCommandWithExitCode()` to use the newly created function.

Additionally, add parameter `timestamp` to function `runCommand()`.

Breaking changes:
* Renamed parameter `nonBlocking` to `discardOutput` in
`runCommandWithExitCode()`.
Increase timeout for kpsewhich path expansion to prevent
LatexPackageLocationCache being empty.

Additionally, make `fillCacheWithKpsewhich()` a suspend function, use
non-blocking shell command execution and add logging to analyze number
of paths loaded into LatexPackageLocationCache.

See !3653 for details and measurements.
Additionally, update usages of `String.runCommand()` in suspending
functions to use `String.runCommandNonBlocking()`.
Copy link
Collaborator

@PHPirates PHPirates left a comment

Choose a reason for hiding this comment

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

Great, thanks a lot! I learned some things about coroutines again.

@PHPirates PHPirates merged commit 6bfc764 into Hannah-Sten:master Nov 12, 2024
17 checks passed
@PHPirates PHPirates added this to the Next milestone Nov 12, 2024
@tristankretzer tristankretzer deleted the issue/3653 branch November 12, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kpsewhich times out on Windows when using texlive-full
2 participants