-
Notifications
You must be signed in to change notification settings - Fork 273
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
Add an LSP server command #3316
Conversation
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
private/buf/buflsp/file_manager.go
Outdated
} | ||
|
||
// newFiles creates a new file manager. | ||
func newFiles(lsp *lsp) *fileManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newFileManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my phone but don't forget changelog if we haven't done it
|
||
func (mu *mutex) storeWho(id uint64) { | ||
for { | ||
// This has to be a CAS loop to avoid races with a poisoning p. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. If you are unable to swap it's confirmed to be poisoned as the lock is always held.
if !mu.who.CompareAndSwap(0, id) {
panic("buflsp/mutex.go: non-reentrant lock locked twice by the same request")
}
mu.storeWho(0) | ||
|
||
mu.pool.check(id, mu, true) | ||
mu.lock.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mu.lock.Unlock()
should be deferred, as this never gets called on panic
of storeWho
. So go routines will still deadlock on line 150
as they wait for this Unlock. Although does this matter? Would only matter if using recovers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcy can you check over all of your concurrency logic here before we merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah -- I think that's the crux of this, it seems to me that panic
's are used under the assumption that we are crashing the program to avoid attempting to recover from bad code-patterns (and thus, hopefully, preventing us from merging bad patterns). That being said, deferring here to make the pattern with storeWho
a little less fragile is probably a good thing.
private/buf/buflsp/file_manager.go
Outdated
// by the editor. | ||
type fileManager struct { | ||
lsp *lsp | ||
table refcount.Map[protocol.URI, file] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doriable can you do a pass on naming here? None of these names match our style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments around style/readability.
This PR adds a new command, buf beta lsp, which serves an LSP for Protobuf files.
Most of the code lives under
buflsp
. Currently, the LSP implements: