sublimehq / sublime_text

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

[ST4] Pass along the `view.change_count()` in `on_text_changed_async` #3685

Open rwols opened 4 years ago

rwols commented 4 years ago

Problem description

Due to the nature of the async callbacks, there is a race condition in determining the change count ("revision") for a given list of incremental text changes.

When the incremental changes arrive in on_text_changed_async, there might be a need to inspect the view.change_count() in order to track a revision of the buffer. It is however possible that on_text_changed_async is called much later than when the user pressed a key. For instance, something in the async thread can sleep for 5 seconds, blocking all pending *_async callbacks.

Preferred solution

To guarantee correct behavior in this area, the signature of on_text_changed_async must be modified to take an integer as well. It should be the change_count of the view at the point in time just after the changes were inserted.

Alternatives

Note that this problem does not occur in the version that runs in the UI thread, on_text_changed. That is because calling view.change_count() while in on_text_changed guarantees the correct change count.

While it is certainly a possibility for plugins to use on_text_changed to circumvent this race condition, it will feel slower for end-users.

deathaxe commented 4 years ago

A plugin can always use on_text_changed() to queue (or drop) events in the UI thread while still using a worker thread to dispatch them. This wouldn't have any impact on front-end performance. The synchronous event handlers that just queue stuff are too quick to even meassure their runtime.

GitGutter listens events in a synchronous EventListener, debounces them and queues the final signal via set_timeout_async(). One could even use a (real) Future or something like that to queue them in a dedicated plugin worker thread. GitGutter has a task.py to maintain its own worker thread to prevent other plugins from being delayed by blocking calls.

ST's async worker thread may not be a good choise for two many plugins performing expensive tasks anyway as they might delay each other. IO-bound tasks would better go into an asyncio loop while CPU bound stuff would still need dedicated threads.

rwols commented 4 years ago

A fair point. Who knows what more plugins may want with respect to the view state. I use on_text_changed now and it all works. Consider this issue a low-priority request.