sublimehq / sublime_text

Issue tracker for Sublime Text
https://www.sublimetext.com
803 stars 39 forks source link

Packages are allowed to hang Sublime Text Indefinitely #1463

Open evandrocoan opened 7 years ago

evandrocoan commented 7 years ago

Packages are allowed to hang Sublime Text Indefinitely

For example, a plugin command call may enter on while( true ) causing Sublime Text to became unresponsive indeterminately forcing the user to kill the process as the only way to fix it.

This behavior can be noticed on several applications as the Mozilla Firefox. But on other applications as Google's Chrome it is not true. The nature around Google's chrome is that everything dangerous is spammed on separate thread, and soon as the Chrome's Task Manager noticed hand/blocking threads, it prompts to the user's whether to kill them or wait.

The simple thing for this is not run unknown procedures on the main Sublime Text process, as any simple thing may hang. Only run certain deterministic procedures on the main Sublime Text process and a create watchdogs for trouble some routines.

Related issues:

  1. https://github.com/SublimeTextIssues/Core/issues/1296 _ file indexer causes hanging on large directories
  2. https://github.com/SublimeTextIssues/Core/issues/763 _ Closing network connection causes ST to permanently hang
  3. #23760/3 _ Automatically set SQL keywords to upper case

Another example is the SQLExec package, if the start a connection to the SGBD and the SGBD prompt for the password. The package was not expecting it, then Sublime Text hands indefinitely doing absolutely nothing as it is waiting for the user to put the password, event which will never happen.

Expected behavior

Is expected that packages are not allowed to hang Sublime Text indefinitely.

Actual behavior

If some package enters on while true or takes too much to perform its actions, Sublime Text may hang indefinitely.

Steps to reproduce

1) Create a package as this:

class DefaultSyntaxCommand(sublime_plugin.EventListener):

    def on_new(self, view):

        view.set_syntax_file("Packages/C++/C++.tmLanguage")

        while True :
            print( 'bitch\n' )

2) Create a new file, and Sublime Text will hang for ever and ever.

keith-hall commented 7 years ago

Firefox has recently been updated so that tabs are rendered on a separate thread and I hate it. Why? Because, before, I used to be able to instantly switch tabs, and now, with electrolysis, I get a loading icon for a second first. :(

So I feel there is a danger that adding functionality to check if a plugin has "stalled" will degrade performance too much. Note that plugins can be profiled using Tools -> Developer -> Profile Plugins, and users are always free to uninstall any plugins that behave undesirably. I've therefore marked this as minor.

evandrocoan commented 7 years ago

From: https://forum.sublimetext.com/t/sublime-text-performance-with-very-large-files/9832/15

Eclipse IDE asks the user whether they want to disable all extra features as syntax highlight, word-wrap and etc, before opening a big file. So the user may choose whether to wait more and get all features, or open early with no extra features. Sublime could do the same.

Yeah, Sublime text has a loading bar, I tested opening 1GB file, and it allowed to cancel closing the file hitting by ctrl+w.

For 1GB file, it required a little more than 1GB of RAM's Memory. But right after filling the progress bar, Sublime Text hanged for 20 some seconds. Then after it became responsive, I tried to interact with the file and Sublime Text hanged using all its CPU.

hang

For now, 5 minutes has passed and it still hanging, so I am killing the process. This 5 minutes hang after opened the file is because of issue #1463 Packages are allowed to hang Sublime Text Indefinitely.

Because when I did the same on a Vanilla Install, Sublime Text only hanged for 20 seconds right after the loading bar got filled. After it It was completely responsive! Good work. Now we know Sublime Text may handle 1GB files, but only the Vanilla version, because if there are more packages installed, they will hang Sublime Text due the issue #1463 Packages are allowed to hang Sublime Text Indefinitely.

FichteFoll commented 7 years ago

This would require some major architecture considerations.

Either way, there are actions that you want to happen synchronously, as @keith-hall indicated, because the user shouldn't see UI changes before a plugin has done work. A good example for this would be the InactivePanes package, where you would see flickering if it was executed asynchronously.

On the other hand, plugins should realize by themselves whether they are running time-critical tasks and should otherwise resort to the *_async hooks, which are not executed on the UI thread. Also, there is only one async thread that all plugins share, which is not ideal (https://github.com/divmain/GitSavvy/issues/421#issuecomment-223148236).

evandrocoan commented 7 years ago

This would require some major architecture considerations.

Yeah, this would bring Sublime Text to a new level. Instead of it be a ordinary program, which run friendly and in hope the packages developed to it are Good Samaritans, it would be robust as an Operational System which run several programs, here known as Packages.

One good way to do it is:

  1. Do not run the plugin stuff on the same thread as the GUI.
  2. Ok, most or some task must to be waited to be completed before the Sublime Text GUI Respond, but is does not mean it must to hang.
  3. Hang is really horrible thing to do. I am not proposing to the Sublime Text team do magic for those long tasks.
  4. When I am saying hang I mean this: hang
  5. When I am saying to not hang I mean this: not_hang

On the other hand, plugins should realize by themselves

  1. I am saying to put loading sign on the status bar or whatever is possible, saying some task is being performed.
  2. Show the user a stack trace to that long task, and allow it to be killed and disable the plugin which is calling it, to avoid the to hang again.
  3. Using this information the package and the user experience could be improved.

Now it is easier to load a 1GB file. You would know what plugin is doing you Sublime Text hang, and in just in time, you would be able to kill the current operation and disable the package, until you stop editing 1GB files, or the package's developer fix the package for heavy load.

deathaxe commented 7 years ago

The nature around Google's chrome is that everything dangerous is spammed on separate thread, and soon as the Chrome's Task Manager noticed hand/blocking threads, it prompts to the user's whether to kill them or wait.

In fact it is a main design principal of Google to do so. In Android nothing else but the GUI is allowed to be handled by the main thread. All expensive (I/O) operations must be handled by extra threads. This is the only way to keep GUI responsive under all circumstances and makes rich animations possible. Especially all I/O operations must not be done in the main thread as each of them may block a thread for seconds, if the IO device needs time to wakeup or so.

And the principle "everything is asynchronous" is the main reason for Node to be so powerful. It uses libuv / libev to access all I/O devices asynchronously with a kind of common OS independent messaging system. This results in very low CPU waits in the thread handling the main_event_loop. But the way an application is to be designed changes dramatically. Each line of synchronous I/O-code results in a separate function to handle the result of a message and fire the next one.

The drawback as @FichteFoll outlines is a high effort to keep track of all that asynchronous tasks and to sync their results into the main thread for GUI updates.

@FichteFoll: To avoid unwanted GUI updates I could think of API functions like begin_update() and end_update() so an plugin can ask the main thread not to perform any GUI changes to a view/window or the whole application. This way plugins are able to perform their time expensive job asynchronous first and then push all required GUI changes to the main thread, so the GUI needs to be redrawn once only. I think of changed visual settings as an example. Imagine a plugin wants to change font size and font face at a time. This is not possible at the moment as GUI redraws after size change and once again after font face change.

The following example should outline a way to handle expensive async jobs, no matter whether the Thread was created by SublimeText or the plugin itself.

def PluginWorker():
    ...
    # do the time expensive job in the background
    ...

    # sync with main thread
    try:
        sublime.begin_update()  # freeze repaint
        view.settings().set("font_size", new_size)
        view.settings().set("font_name", new_name)
        ...
    finally:
        sublime.end_update()    # enable and do repaint

I know about the really hard job to handle async operations, but I agree with @evandrocoan Sublime Text should not loose control to one or more unresponsive plugins. It may not be useful to run each plugin in its own thread but at least to run them all in one seperate one, to keep GUI responsive and enable SublimeText to kill an unresponsive task.

evandrocoan commented 7 years ago

to keep GUI responsive and enable SublimeText to kill an unresponsive task.

deathaxe commented 7 years ago

On the other hand, plugins should realize by themselves whether they are running time-critical tasks and should otherwise resort to the *_async hooks, which are not executed on the UI thread.

The next release of GitGutter will be a good example to learn, how to correctly run time-critical tasks asynchronously, from plugin side. All of the steps required to build the gutter icons (call git -> call diff -> ...) are chained by a Promise object. It makes handling asynchronous tasks quite easy.

The main entry point to run each type of task async is by starting it with sublime.set_timeout_async(...) returning a Promise.

The plugin interferes with main thread only very little which results in no more lags when typing.

The conclusion is: Maybe this type of async task handling could become part of the sublime_api as base for other plugin developers, too. This would avoid deep changes in the core code and could provide everything to run time critical tasks safely without loosing control.

I also had a look on python's default library and found concurrent.futures which is basically the same.

This leaves only one question: How to ensure plugin developers make use of those features to ensure quality of service?

keith-hall commented 7 years ago

For reference, this is the PR @deathaxe is referring to re GitGutter promises: https://github.com/jisaacks/GitGutter/pull/321