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

AutoImport can break when being called by multiple threads #474

Closed
tkrabel opened this issue Oct 28, 2023 · 1 comment · Fixed by #498
Closed

AutoImport can break when being called by multiple threads #474

tkrabel opened this issue Oct 28, 2023 · 1 comment · Fixed by #498
Labels
bug Something isn't working
Milestone

Comments

@tkrabel
Copy link
Contributor

tkrabel commented Oct 28, 2023

What is the problem?

In python-rope/rope#713, I explain that AutoImport APIs can only be called from the thread that created it. This causes issues when pylsp is using multithreading, as it does when launched with the --ws argument.

As of now, we haven't seen this issue being reported, but I can replicate it in a testing environment, when calling workspace.close() (as happens on e.g. m_shutdown). My current solution of using one AutoImport object per rope_autoimport feature (#471) mitigates this issue, but won't solve it.

What are potential solution options?

I considered three options

  1. Exposing check_same_thread by AuoImport and setting AutoImport(chech_same_thread=False):
    sqlite3 allows creating connections, setting check_same_thread=False (default True), which turns off thread checks. This would solve not running into sqlite thread check errors, and sqlite seems to be threadsafe but we're highly discouraged from using multithreads with sqlite. Also, check_same_thread is currently not exposed by AutoImport, and there is pushback against doing that (Allow sqlite to be accessed from different thread python-rope/rope#714 (comment)). This may make patching AutoImport necessary, which is what I'd abstain from.
  2. Use one AutoImport object per thread:
    This seems a valid approach in the beginning, but leaves questions like "how we close all data base connections on m_shutdown?" unanswered. I don't know how to solve that from within a thread pool without passing the executor to every request, and that would couple the language server with a concurrency detail. But maybe this is not an issue? I don't know.
  3. Use always the same worker thread for API calls on AutoImport:
    To make operations threadsafe while supporting using a thread pool, we can proxy all autoimport API calls through a separate thread pool with one worker thread. This makes autoimport calls a bottleneck, but since calls to it are very fast, this shouldn't be too much of a problem.
  4. Upstream change: create one connection per thread using a thread local: Create one database connection per thread python-rope/rope#720
    We can let rope swallow the multithreading complexity while avoiding artificial bottlenecks like in (3). This would require bumping our requirements though, which is something I am OK with.

What is my recommendation?

If there are no objections, I would go with option (4). Option (1) may non-deterministically corrupt the autoimport database, which is extremely hard to debug, and (2) may leave dangling database connectors. (3)'s performance degradation is negligible I think, but I prefer solving this upstream.

@ccordoba12
Copy link
Member

If there are no objections, I would go with option (4).

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants