CodeEditApp / CodeEditSourceEditor

A code editor view written in Swift powered by tree-sitter.
https://codeeditapp.github.io/CodeEditSourceEditor/documentation/codeeditsourceeditor
MIT License
514 stars 80 forks source link

Rework Async `tree-sitter` Model, Fix Strong Ref Cycle #225

Closed thecoolwinter closed 8 months ago

thecoolwinter commented 8 months ago

Description

In more detail: The current async tree-sitter implementation uses a mix of Swift Tasks, pthread locks and a custom task scheduler to perform highlight operations asynchronously. This is dangerous for a variety of reasons, so the TreeSitterClient object has been reworked.

Instead of a class, it is now an actor. Meaning it's still a reference type, but now all asynchronous accesses are performed serially using swift's process pool. This removes the need for any custom locking mechanism, and reduces the likelihood of data races and deadlocks. This also means all operations on the client must be performed asynchronously, so the HighlightProviding protocol has been updated to use async functions in place of callbacks. Finally, the Highlighter class has been updated to keep a set of currently running tasks and provides a method to cancel them if needed.

The client has been reworked to use a single DispatchQueue to coordinate serial access to the client. This allows the object to perform an operation either synchronously or asynchronously depending on a variety of factors. In addition to dispatching all operations to a serial queue, we enforce usage of the client from the main queue to ensure all method calls are performed serially.

I did mention in my comment that I wanted to use locks, but ended up not needing any due to the use of serial queues.

This change is an improvement for two reasons:

The strong reference cycle occurred in the Coordinator for CodeEditSourceEditor, which was keeping an optional (but not weak) reference to the text view controller. This has been made weak, correctly referencing the controller. There was also a strong reference cycle between Highlighter and the text view controller that has been corrected.

Related Issues

Checklist

thecoolwinter commented 8 months ago

Actually, I'd like to rethink using actors here. I'm going to mark this as a draft, and open PRs for the few small bugs fixed in this one.

My thinking is:

The first point is a serious limitation for things like filters, that need to run synchronously. For instance if we wanted an HTML completion filter that fills in tags using tree-sitter information. That filter needs a synchronous API and tree-sitter is fast enough to perform the necessary queries in time for an operation like that. But, if an actor is used we cannot perform that operation synchronously.

Instead, the tree-sitter operation should be attempted synchronously. If it is determined it may take too long to perform on the main thread an error is thrown and the API caller must either re-call the API using the async version or return.

The API would look like:

// Somewhere that needs tree sitter information
let client = TreeSitterClient(...)

do {
    try client.withLayerSync(...) { [weak self] in
        // use captured language layer
    }
} catch {
    if let error = error as? TreeSitterClientError {
        switch error {
            case .notSyncSafe: ...
                // retry with async call
                client.getLayerAsync { [weak self] in
                    // use captured language layer.
                }
        }
    }
}

This will require a locking mechanism that will be handled by the client, but because this system wouldn't use Swift's concurrency features like before I feel much safer using OS locks.

If anyone has some thoughts please post them here!