Repository navigation
Conversation
836ce5c to
59cdf87
Compare
4a1f066 to
2fd5280
Compare
2fd5280 to
fd458d0
Compare
d251b46 to
48037ae
Compare
|
@Lotes I identified a few more issues in the |
978c1e2 to
57447e2
Compare
^ Conflicts: ^ server/server.go ^ workspace/builder.go ^ Conflicts: ^ cmd/fastbelt/generate.go ^ workspace/builder.go
57447e2 to
a59d2bf
Compare
sailingKieler
left a comment
sailingKieler
left a comment
There was a problem hiding this comment.
Thank you Mark! I like this approach much better, never fully understood the downgrade idea.
I have a couple of hints for better naming & documentation and one critical question in the last remark, see below. Hope this helps.
| // | ||
| // It is safe to call concurrently with a running build: the atomic load pairs | ||
| // with [Document.SetState], so a caller that observes a state bit is | ||
| // guaranteed to see all document data written by that build phase. |
There was a problem hiding this comment.
A nice AI generated comment 😄
, so a caller that observes a state bit is guaranteed to see all document data written by that build phase.
This is rather an assertion the impl hopefully satisfies instead of a consequence.
I suggest to replace , so a by . A.
| // [Lock.ReadAt] requests are admitted as soon as the states they need are reached, | ||
| // and report the workspace-wide floor after each phase barrier. The floor relies | ||
| // on every document outside docs already being complete, which the caller must | ||
| // ensure (the updater collects every incomplete document into the build set). |
There was a problem hiding this comment.
This doc is very AI compact. Not sure whether I really get it or rather project my expectation onto the statement...
| } | ||
| if ctx.Err() != nil { | ||
| return | ||
| if ctx.Err() != nil { |
There was a problem hiding this comment.
ctx.Err() != nil usually indicates (self) cancellation or the current run, correct?
I think that is worth a comment at the beginning of the method.
| if err := ctx.Err(); err != nil { | ||
| return err | ||
| } | ||
| // Phase 2 barrier: the whole workspace has its static scopes computed. |
There was a problem hiding this comment.
| // Phase 2 barrier: the whole workspace has its static scopes computed. | |
| // Phase 2 barrier release: the whole workspace has its static scopes computed. |
| writeHeld bool // exclusive write phase is active | ||
| writeWaiters int // number of goroutines waiting to acquire the write lock | ||
| readers int // number of active shared lock holders (Read and ReadAt) | ||
| stateAdvanced bool // current write entered the build phase; document states only advance |
There was a problem hiding this comment.
Maybe
| stateAdvanced bool // current write entered the build phase; document states only advance | |
| stateAdvancing bool // current write entered the build phase; document states only advance |
| mu sync.Mutex | ||
| cond *sync.Cond | ||
| writeHeld bool // exclusive write phase is active | ||
| writeWaiters int // number of goroutines waiting to acquire the write lock |
There was a problem hiding this comment.
despite the symmetry to writeHeld writeWaiters is a weird word, waiters work in restaurants.
| writeWaiters int // number of goroutines waiting to acquire the write lock | |
| waitingWriters int // number of goroutines waiting to acquire the write lock |
|
|
||
| mu sync.Mutex | ||
| cond *sync.Cond | ||
| writeHeld bool // exclusive write phase is active |
There was a problem hiding this comment.
Maybe just writing
| writeHeld bool // exclusive write phase is active | |
| writing bool // exclusive write phase is active |
| }() | ||
|
|
||
| do(ctx, downgrade) | ||
| do(ctx) |
There was a problem hiding this comment.
My gut feeling says that some
if ctx.Err() != nil {
// current write task is canceled
return
}
do(ctx)
would complete the disposal of older write jobs, although this is done within do, at least by the builder.
At least it documents how abortion of pending writers is actually performed.
| if len(docs) == 0 { | ||
| return nil | ||
| } | ||
| return builder.Build(ctx, docs) |
There was a problem hiding this comment.
Critical: Why is this call of builder.Build(ctx, docs) not protected by a write lock?
Early didChange and documentSymbols requests by lang clients may race with the initial workspace build, don't they?
Besides in Langium we have the workspace ready promise
I think we should have <-chan struct{} singleton instance serving the same purpose.
This PR attempts to fix a similar problem as our implementation already does inside of Langium. Some services require only specific document states to be available. For example, a document symbol request can be immediately resolved, once the document has been succesfully parsed - without waiting for the build to finish, or even references to be linked.
This change adds two main features for this:
ReadAtmethod to theLockinterface, which allows to read documents at specific states.DocumentStateRequirementsinterface, that LSP services can (optionally) implement to start reading at a specific workspace/document state. Any service that does not implement this interface falls back toRead.Removes the
downgradefunctionality from the lock, which has been replaced withReadAt.WriteandReadare now fully exclusive.Related to eclipse-langium/langium#2214.