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

[ST4] Allow plugin_unloaded to return a promise/future #3686

Open
rwols opened this issue Oct 10, 2020 · 8 comments
Open

[ST4] Allow plugin_unloaded to return a promise/future #3686

rwols opened this issue Oct 10, 2020 · 8 comments

Comments

@rwols
Copy link

rwols commented Oct 10, 2020

Problem description

If a plugin makes heavy use of the async (worker) thread of sublime text, it can be the case that certain data structures of that plugin must be removed/cleaned up in the worker thread too, due to the design of such a plugin.

It is currently not possible to notify the plugin_host exactly when those data structures are cleaned up in the worker thread.

Consequently, when saving a python file at the root of a package directory, plugin_unloaded is invoked, and then immediately after that, plugin_loaded is invoked. This forces a plugin to unload its data structures in the UI thread in plugin_unloaded.

Preferred solution

Allow a plugin to return a promise/future from plugin_unloaded:

def plugin_unloaded() -> sublime.Future:
    future = sublime.Future()
    
    def unload_async() -> None:
        global my_data_structure
        my_data_structure.clear()

        def resolve_from_main_thread() -> None:
            future.set_result(None)

        sublime.set_timeout(resolve_from_main_thread)

    sublime.set_timeout_async(unload_async)
    return future

Note that this sublime.Future class doesn't actually exist. It is a hypothetical monadic future.

The sublime_plugin.py file should then check if plugin_unloaded returns a future, and if so, attach a continuation to it. Only when the future resolves should the plugin be reloaded.

Alternatives

N/A

Additional Information (optional)

Highly related is the thread about providing a global asyncio event loop: #3129

If SublimeHQ were to implement an event loop, it's likely a plugin would need to do asynchronous cleanup of its datastructures. In that case, plugin_unloaded would have to could optimally be async:

async def plugin_unloaded() -> None:
    global my_tcp_server
    my_tcp_server.close()
    await my_tcp_server.wait_closed()

(closing a tcp server is an asynchronous operation)

@deathaxe
Copy link
Collaborator

I wonder if it was possible to just add a command to plugin_unloaded that simply blocks the function until all threads are done. Something like the join command.

@deathaxe
Copy link
Collaborator

Returning a promise by plugin_reloaded() would mean to implement plugin loading/unloading/reloading in an asynchronous way using asyncio or something like that.

This feels like massivly breaking backward compatibility.

@rwols
Copy link
Author

rwols commented Oct 18, 2020

No this can be backwards compatible I think.

The code in sublime_plugin.py can do the following checks:

  1. Is plugin_unloaded a coroutine function? There is asyncio.iscoroutinefunction to inspect that. If so, schedule that coroutine function on a loop, await when it is done, and only then call plugin_loaded.
  2. If it's not a coroutine function, call plugin_loaded and inspect the return type. If the return type is None, go ahead and call plugin_loaded (this is the current behavior). If the return type is this hypothetical Future type, schedule the future and attach a done_callback to it. When the done_callback is invoked, call plugin_loaded.

@deathaxe
Copy link
Collaborator

... which could basically be done with any API function in the one way or the other which results in overcomplicating everything.

Turning plugin_unloaded in an optional coroutine would have massive impact on the whole plugin (un-/re-)loading backbone. The only benifit of scheduling plugin_unloaded as coroutine and implementing the whole plugin loading machinary via asyncio might only be pseudo-parallel reloading several plugins at a time.

IMHO, without turning the whole API into asyncio plugin_unloaded should just block until a plugin has cleaned everything up and closed all threads to keep the synchronous nature of the current API intact.

There's a little bit more going on than just delaying a following plugin_loaded in case of the one special case of reloading an active plugin.

@wbond
Copy link
Member

wbond commented Oct 18, 2020

My personal opinion would be to avoid adding async in various places in the API, mostly because I think that such APIs don't lead to better code that is easier to reason about, and implementation wise tend to be more complicated. I don't feel particularly strongly about this specific situation.


If I were to approach this problem right now, I would probably something like:

  1. In the blocking plugin_unloaded(), do the following:
  • Request any necessary values from the API
  • Copy any necessary data from disk or other variables about what needs to be cleaned up
  • Poison any pending items in the worker thread
  • Start a thread to clean up files, shut down processes, etc
  1. Let Sublime Text unload the plugin and delete references

Because of garbage collection, the thread will keep running doing its cleanup, but won't get in the way of modules being reloaded, or Sublime Text doing its thing. Once it is complete, the thread will end and all resources will be cleaned up/reaped.

This would allow processes to be killed or shut down, it allows it to do expensive file system operations like cleaning up cache directories.

It may be that a plugin needs to appropriately scope file names so they are "session"-specific and it is possible for a server to be in the process of being shut down, while another one is being started.

In terms of the worker thread, items there have a few options. They could hold a reference to a variable that is poisoned during shutdown, or they could query the current generation of the plugin running in memory.

@rwols
Copy link
Author

rwols commented Oct 18, 2020

What you describe cannot work. It may be that a TCP server has a listener socket on a specific port, let's say 8080. If you were to clean up that server in a separate thread, then the plugin_loaded call would try to start another TCP server at port 8080, which is still in use by the "poisoned" TCP server that is about to be cleaned up.

The point of this issue is to reach some sort of synchronization between plugin unloads and loads. While I wrote an example of using async def, plugins can of course wrap the hypothetical sublime.Future and resolve it via their own means. That could be via starting a thread with threading.Thread as you describe, or by scheduling it on some ambient asyncio.AbstractEventLoop as I propose.

So, the core problem is synchronization.

Really, the same problem is present in EventsListener.on_exit. For that callback, there is no way to asynchronously clean up resources. I cannot return a sublime.Future, only None.

@wbond
Copy link
Member

wbond commented Oct 18, 2020

You could write your server to not use a specific port, or you could have a way to have to stop listening during cleanup.

on_exit is the same sort of thing. You just need to write your code/ infrastructure in such a way that it gets sent a signal and takes care of itself, rather than needing to block things.

When it comes down to it, this is a wish for Sublime Text to change a bunch of APIs to be asynchronous because plugins want to be blocking and not asynchronous in their own responsibilities.

@rchl
Copy link

rchl commented Jun 10, 2021

on_exit is the same sort of thing. You just need to write your code/ infrastructure in such a way that it gets sent a signal and takes care of itself, rather than needing to block things.

Sending a signal wouldn't work because the code that would have to run to process that signal is gonna get killed on ST exit.
That said, letting plugins block the exit wouldn't be ideal either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants