internal/lsp: fix deadlocks loading lots of files at once #129
Conversation
The first deadlock involved differing mutex acquisition order in two code paths: 1. loadParseTypecheck() holds the "mcache" mutex then eventually acquires the "handleMu" file mutex. 2. (*goFile).invalidateContent() acquires the "handleMu" mutex first and then the "mcache" mutex. Fix by changing the acquisition order in invalidateContent(). The second deadlock involved the file watcher. The two code paths involved were: 1. (*goFile).GetPackages() holds the view mutex and eventually gets (*WatchMap).Watch, which acquires the watcher mutex. 2. (*session).openOverlay acquires the watcher mutex as it triggers a file's callbacks, and then the callback "(*goFile).invalidateContent" acquires the vie mutex. Fix by not holding the watcher mutex as we invoke the callbacks.
|
This PR (HEAD: 04944fa) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/tools/+/184880 to see it. Tip: You can toggle comments from me using the |
|
Message from Rebecca Stambler: Patch Set 2: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/184880. |
|
Message from Rebecca Stambler: Patch Set 2: Run-TryBot+1 Thank you! Please don’t reply on this GitHub thread. Visit golang.org/cl/184880. |
|
Message from Gobot Gobot: Patch Set 2: TryBots beginning. Status page: https://farmer.golang.org/try?commit=59c25da1 Please don’t reply on this GitHub thread. Visit golang.org/cl/184880. |
|
Message from Gobot Gobot: Patch Set 2: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/184880. |
The first deadlock involved differing mutex acquisition order in two code paths: 1. loadParseTypecheck() holds the "mcache" mutex then eventually acquires the "handleMu" file mutex. 2. (*goFile).invalidateContent() acquires the "handleMu" mutex first and then the "mcache" mutex. Fix by changing the acquisition order in invalidateContent(). The second deadlock involved the file watcher. The two code paths involved were: 1. (*goFile).GetPackages() holds the view mutex and eventually calls (*WatchMap).Watch, which acquires the watcher mutex. 2. (*session).openOverlay acquires the watcher mutex as it triggers a file's callbacks, and then the callback "(*goFile).invalidateContent" acquires the view mutex. Fix by not holding the watcher mutex as we invoke the callbacks. Fixes golang/go#32910 Change-Id: I9d060e0d80fd86a317a1d6c7aaa736a8ce10bd07 GitHub-Last-Rev: 04944fa GitHub-Pull-Request: #129 Reviewed-on: https://go-review.googlesource.com/c/tools/+/184880 Reviewed-by: Rebecca Stambler <rstambler@golang.org> Run-TryBot: Rebecca Stambler <rstambler@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
This PR is being closed because golang.org/cl/184880 has been merged. |
The first deadlock involved differing mutex acquisition order in two code paths: 1. loadParseTypecheck() holds the "mcache" mutex then eventually acquires the "handleMu" file mutex. 2. (*goFile).invalidateContent() acquires the "handleMu" mutex first and then the "mcache" mutex. Fix by changing the acquisition order in invalidateContent(). The second deadlock involved the file watcher. The two code paths involved were: 1. (*goFile).GetPackages() holds the view mutex and eventually calls (*WatchMap).Watch, which acquires the watcher mutex. 2. (*session).openOverlay acquires the watcher mutex as it triggers a file's callbacks, and then the callback "(*goFile).invalidateContent" acquires the view mutex. Fix by not holding the watcher mutex as we invoke the callbacks. Fixes golang/go#32910 Change-Id: I9d060e0d80fd86a317a1d6c7aaa736a8ce10bd07 GitHub-Last-Rev: 04944fa GitHub-Pull-Request: golang#129 Reviewed-on: https://go-review.googlesource.com/c/tools/+/184880 Reviewed-by: Rebecca Stambler <rstambler@golang.org> Run-TryBot: Rebecca Stambler <rstambler@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
The first deadlock involved differing mutex acquisition order in two
code paths:
acquires the "handleMu" file mutex.
then the "mcache" mutex.
Fix by changing the acquisition order in invalidateContent().
The second deadlock involved the file watcher. The two code paths
involved were:
(*WatchMap).Watch, which acquires the watcher mutex.
file's callbacks, and then the callback
"(*goFile).invalidateContent" acquires the view mutex.
Fix by not holding the watcher mutex as we invoke the callbacks.
Fixes golang/go#32910