From 68c5cd8a556343ba325f2da637a9de200be54ac5 Mon Sep 17 00:00:00 2001 From: Ned Palacios Date: Mon, 1 Apr 2024 17:14:19 +0800 Subject: [PATCH] feat: remove documents in lsp server, follow single truth principle by relying on daemon server --- server/daemon/client/client.go | 8 ++ server/daemon/server/server.go | 105 ++++++++++++++++++++++++--- server/daemon/server/server_test.go | 47 ++++++++++++ server/daemon/types/methods.go | 7 +- server/lsp_server/lsp_server.go | 27 ++----- server/lsp_server/lsp_server_test.go | 46 ++++++------ 6 files changed, 182 insertions(+), 58 deletions(-) diff --git a/server/daemon/client/client.go b/server/daemon/client/client.go index 309b08b..b1d38f1 100644 --- a/server/daemon/client/client.go +++ b/server/daemon/client/client.go @@ -264,6 +264,14 @@ func (c *Client) DeleteDocument(filepath string) error { }, nil) } +func (c *Client) RetrieveDocument(filepath string) (string, error) { + var content types.DocumentPayload + err := c.Call(types.RetrieveDocumentMethod, types.DocumentIdentifier{ + Filepath: filepath, + }, &content) + return content.Content, err +} + func (c *Client) GetDataDirPath() (string, error) { var path string err := c.Call(types.GetDataDirMethod, nil, &path) diff --git a/server/daemon/server/server.go b/server/daemon/server/server.go index 55a366b..4884926 100644 --- a/server/daemon/server/server.go +++ b/server/daemon/server/server.go @@ -34,6 +34,8 @@ type resultError struct { type Server struct { ServerLog *log.Logger engine *errgoengine.ErrgoEngine + // fileUseCounter is use to keep track how many clients are using a file + fileUseCounter map[string][]int // TODO: add storage for context data connectedClients connectedClients logger *logger.Logger @@ -203,11 +205,21 @@ func (d *Server) Handle(ctx context.Context, c *jsonrpc2.Conn, r *jsonrpc2.Reque return } - if err := d.FS().WriteFile(payloadStr.Filepath, []byte(payloadStr.Content)); err != nil { - c.ReplyWithError(ctx, r.ID, &jsonrpc2.Error{ - Message: err.Error(), - }) - return + if _, ok := d.fileUseCounter[payloadStr.Filepath]; !ok { + if err := d.FS().WriteFile(payloadStr.Filepath, []byte(payloadStr.Content)); err != nil { + c.ReplyWithError(ctx, r.ID, &jsonrpc2.Error{ + Message: err.Error(), + }) + return + } + + d.fileUseCounter[payloadStr.Filepath] = []int{} + } + + // check if the current connected client is present in specific file of fileUseCounter + procId, _ := d.getProcessId(r) + if idx := d.GetFileUseIdx(payloadStr.Filepath, procId); idx == -1 { + d.fileUseCounter[payloadStr.Filepath] = append(d.fileUseCounter[payloadStr.Filepath], procId) } d.ServerLog.Printf("resolved document: %s (len: %d)\n", payloadStr.Filepath, len(payloadStr.Content)) @@ -277,18 +289,76 @@ func (d *Server) Handle(ctx context.Context, c *jsonrpc2.Conn, r *jsonrpc2.Reque file.Close() } - // TODO: use dependency tree - if err := d.FS().Remove(payload.Filepath); err != nil { + // decide if the file will be removed + procId, _ := d.getProcessId(r) + if idx := d.GetFileUseIdx(payload.Filepath, procId); idx != -1 { + // remove the process id from the file use counter + d.fileUseCounter[payload.Filepath] = append( + d.fileUseCounter[payload.Filepath][:idx], + d.fileUseCounter[payload.Filepath][idx+1:]...) + + if idx > 0 { + d.ServerLog.Printf("file %q is still in use. removing the client from the file users instead", payload.Filepath) + } + } + + if fileConsumers, ok := d.fileUseCounter[payload.Filepath]; !ok || len(fileConsumers) == 0 { + if ok { + delete(d.fileUseCounter, payload.Filepath) + } + + if err := d.FS().Remove(payload.Filepath); err != nil { + c.ReplyWithError(ctx, r.ID, &jsonrpc2.Error{ + Message: err.Error(), + }) + return + } + + d.ServerLog.Printf("removed document: %s\n", payload.Filepath) + } + + c.Reply(ctx, r.ID, "ok") + + // doc := d.engine + case types.RetrieveDocumentMethod: + var payload types.DocumentIdentifier + if err := json.Unmarshal(*r.Params, &payload); err != nil { + c.ReplyWithError(ctx, r.ID, &jsonrpc2.Error{ + Message: "Unable to decode params of method " + r.Method, + }) + return + } + + if len(payload.Filepath) == 0 { + c.ReplyWithError(ctx, r.ID, &jsonrpc2.Error{ + Message: "Filepath is empty", + }) + return + } + + file, err := d.FS().Open(payload.Filepath) + if err != nil { c.ReplyWithError(ctx, r.ID, &jsonrpc2.Error{ Message: err.Error(), }) + return + } else { + file.Close() } - d.ServerLog.Printf("removed document: %s\n", payload.Filepath) - c.Reply(ctx, r.ID, "ok") + fileContents, err := d.FS().ReadFile(payload.Filepath) + if err != nil { + c.ReplyWithError(ctx, r.ID, &jsonrpc2.Error{ + Message: err.Error(), + }) + return + } - // doc := d.engine + c.Reply(ctx, r.ID, types.DocumentPayload{ + DocumentIdentifier: payload, + Content: string(fileContents), + }) case types.RetrieveParticipantIdMethod: c.Reply(ctx, r.ID, d.logger.ParticipantId()) case types.GenerateParticipantIdMethod: @@ -483,6 +553,20 @@ func (s *Server) Start(addr string) error { ) } +func (s *Server) GetFileUseIdx(file string, procId int) int { + if _, ok := s.fileUseCounter[file]; !ok { + return -1 + } + + for i, id := range s.fileUseCounter[file] { + if id == procId { + return i + } + } + + return -1 +} + func NewServer() *Server { server := &Server{ ServerLog: log.New(os.Stdout, "server> ", 0), @@ -498,6 +582,7 @@ func NewServer() *Server { OutputGen: &errgoengine.OutputGenerator{}, }, connectedClients: connectedClients{}, + fileUseCounter: map[string][]int{}, errors: []resultError{}, logger: logger.NewMemoryLoggerPanic(), } diff --git a/server/daemon/server/server_test.go b/server/daemon/server/server_test.go index 193307c..d812b42 100644 --- a/server/daemon/server/server_test.go +++ b/server/daemon/server/server_test.go @@ -233,6 +233,10 @@ func TestDeleteDocument(t *testing.T) { t.Fatal(err) } + if srv.GetFileUseIdx("hello.py", clientId) == -1 { + t.Fatalf("expected client to be added as user of the file") + } + // wait for the server to process the document time.Sleep(100 * time.Millisecond) @@ -242,6 +246,11 @@ func TestDeleteDocument(t *testing.T) { t.Fatal(err) } + // check if the client is removed as user of the file + if srv.GetFileUseIdx("hello.py", clientId) != -1 { + t.Fatalf("expected client to be removed as user of the file") + } + // wait for the server to process the document time.Sleep(100 * time.Millisecond) @@ -342,6 +351,10 @@ func TestUpdateDocument(t *testing.T) { t.Fatal(err) } + if srv.GetFileUseIdx("hello.py", clientId) == -1 { + t.Fatalf("expected client to be added as user of the file") + } + // wait for the server to process the document time.Sleep(100 * time.Millisecond) @@ -419,6 +432,40 @@ func TestUpdateDocument_Nonexisting(t *testing.T) { } } +func TestRetrieveDocument(t *testing.T) { + clientId := 1 + conn, srv, client := Setup() + defer conn.Close() + + client.SetId(clientId) + defer client.Close() + + if err := client.Connect(); err != nil { + t.Fatal(err) + } + + // add first document + expected := "print(a)" + err := client.ResolveDocument("hello.py", expected) + if err != nil { + t.Fatal(err) + } + + if srv.GetFileUseIdx("hello.py", clientId) == -1 { + t.Fatalf("expected client to be added as user of the file") + } + + // retrieve the document + resp, err := client.RetrieveDocument("hello.py") + if err != nil { + t.Fatal(err) + } + + if resp != expected { + t.Fatalf("expected non-empty content, got %s", resp) + } +} + func TestCollect(t *testing.T) { clientId := 1 conn, _, client := Setup() diff --git a/server/daemon/types/methods.go b/server/daemon/types/methods.go index d4f3d57..dbbb80f 100644 --- a/server/daemon/types/methods.go +++ b/server/daemon/types/methods.go @@ -44,9 +44,10 @@ var ( // document methods var ( - ResolveDocumentMethod = documentsNamespace.methodName("resolve") - UpdateDocumentMethod = documentsNamespace.methodName("update") - DeleteDocumentMethod = documentsNamespace.methodName("delete") + ResolveDocumentMethod = documentsNamespace.methodName("resolve") + UpdateDocumentMethod = documentsNamespace.methodName("update") + DeleteDocumentMethod = documentsNamespace.methodName("delete") + RetrieveDocumentMethod = documentsNamespace.methodName("retrieve") ) // client methods diff --git a/server/lsp_server/lsp_server.go b/server/lsp_server/lsp_server.go index 080941b..32e3b29 100644 --- a/server/lsp_server/lsp_server.go +++ b/server/lsp_server/lsp_server.go @@ -16,7 +16,6 @@ import ( "github.com/nedpals/bugbuddy/server/release" "github.com/nedpals/bugbuddy/server/rpc" "github.com/nedpals/bugbuddy/server/runner" - "github.com/nedpals/bugbuddy/server/types" "github.com/sourcegraph/jsonrpc2" lsp "go.lsp.dev/protocol" "go.lsp.dev/uri" @@ -42,7 +41,6 @@ type LspServer struct { unpublishedDiagnostics map[uri.URI][]daemonTypes.ErrorReport publishChan chan int doneChan chan int - documents map[uri.URI]*types.Rope } func mustDecodePayload[T any](ctx context.Context, c *jsonrpc2.Conn, r *jsonrpc2.Request) *T { @@ -157,10 +155,9 @@ func (s *LspServer) Handle(ctx context.Context, c *jsonrpc2.Conn, r *jsonrpc2.Re return } - s.documents[payload.TextDocument.URI] = types.NewRope(payload.TextDocument.Text) s.daemonClient.ResolveDocument( payload.TextDocument.URI.Filename(), - s.documents[payload.TextDocument.URI].ToString(), + payload.TextDocument.Text, ) s.publishChan <- len(s.unpublishedDiagnostics) @@ -170,31 +167,20 @@ func (s *LspServer) Handle(ctx context.Context, c *jsonrpc2.Conn, r *jsonrpc2.Re return } - text := s.documents[payload.TextDocument.URI] - // edit the existing text (if any), and send the newly edited version to the daemon for _, change := range payload.ContentChanges { - startOffset := text.OffsetFromPosition(change.Range.Start) - - if len(change.Text) == 0 { - endOffset := text.OffsetFromPosition(change.Range.End) - text.Delete(startOffset, endOffset-startOffset) - } else { - text.Insert(startOffset, change.Text) - } + // NOTE: change the code if TextDocumentKind is set to Incremental + s.daemonClient.UpdateDocument( + payload.TextDocument.URI.Filename(), + change.Text, + ) } - - s.daemonClient.UpdateDocument( - payload.TextDocument.URI.Filename(), // TODO: - text.ToString(), - ) case lsp.MethodTextDocumentDidClose: payload := mustDecodePayload[lsp.DidCloseTextDocumentParams](ctx, c, r) if payload == nil { return } - delete(s.documents, payload.TextDocument.URI) s.daemonClient.DeleteDocument( payload.TextDocument.URI.Filename(), ) @@ -378,7 +364,6 @@ func Start() error { unpublishedDiagnostics: map[uri.URI][]daemonTypes.ErrorReport{}, publishChan: make(chan int), doneChan: doneChan, - documents: map[uri.URI]*types.Rope{}, version: release.Version(), } diff --git a/server/lsp_server/lsp_server_test.go b/server/lsp_server/lsp_server_test.go index 44ad799..9997b35 100644 --- a/server/lsp_server/lsp_server_test.go +++ b/server/lsp_server/lsp_server_test.go @@ -12,7 +12,6 @@ import ( "github.com/nedpals/bugbuddy/server/daemon/server" daemonTypes "github.com/nedpals/bugbuddy/server/daemon/types" "github.com/nedpals/bugbuddy/server/rpc" - "github.com/nedpals/bugbuddy/server/types" "github.com/sourcegraph/jsonrpc2" lsp "go.lsp.dev/protocol" "go.lsp.dev/uri" @@ -44,7 +43,6 @@ func Setup() (func(), *LspServer, *rpc.Client) { unpublishedDiagnostics: map[uri.URI][]daemonTypes.ErrorReport{}, publishChan: make(chan int), doneChan: make(chan int), - documents: map[uri.URI]*types.Rope{}, version: "1.0", } @@ -233,13 +231,13 @@ func TestMethodTextDocumentDidOpen(t *testing.T) { <-srv.publishChan - if _, ok := srv.documents[uri.URI("file:///test.py")]; !ok { - t.Error("Expected document to be opened") - } + // if _, ok := srv.documents[uri.URI("file:///test.py")]; !ok { + // t.Error("Expected document to be opened") + // } } func TestMethodTextDocumentDidOpen_UnsupportedFile(t *testing.T) { - close, srv, client := Setup() + close, _, client := Setup() defer close() _, err := initialize(client) @@ -257,9 +255,9 @@ func TestMethodTextDocumentDidOpen_UnsupportedFile(t *testing.T) { t.Fatal(err) } - if _, ok := srv.documents[uri.URI("file:///test.py")]; ok { - t.Error("Expected document to not be opened") - } + // if _, ok := srv.documents[uri.URI("file:///test.py")]; ok { + // t.Error("Expected document to not be opened") + // } } func TestMethodTextDocumentDidOpen_NoPayload(t *testing.T) { @@ -299,9 +297,9 @@ func TestMethodTextDocumentDidChange(t *testing.T) { <-srv.publishChan - if _, ok := srv.documents[uri.URI("file:///test.py")]; !ok { - t.Error("Expected document to be opened") - } + // if _, ok := srv.documents[uri.URI("file:///test.py")]; !ok { + // t.Error("Expected document to be opened") + // } // change document err = client.Notify(lsp.MethodTextDocumentDidChange, lsp.DidChangeTextDocumentParams{ @@ -349,13 +347,13 @@ func TestMethodTextDocumentDidChange(t *testing.T) { // Wait for the document to be changed time.Sleep(100 * time.Millisecond) - if _, ok := srv.documents[uri.URI("file:///test.py")]; !ok { - t.Error("Expected document to be changed") - } + // if _, ok := srv.documents[uri.URI("file:///test.py")]; !ok { + // t.Error("Expected document to be changed") + // } - if srv.documents[uri.URI("file:///test.py")].ToString() != "print('package main2')" { - t.Errorf("Expected %v, got %v", "print('package main')", srv.documents[uri.URI("file:///test.py")].ToString()) - } + // if srv.documents[uri.URI("file:///test.py")].ToString() != "print('package main2')" { + // t.Errorf("Expected %v, got %v", "print('package main')", srv.documents[uri.URI("file:///test.py")].ToString()) + // } } func TestMethodTextDocumentDidChange_NoPayload(t *testing.T) { @@ -395,9 +393,9 @@ func TestMethodTextDocumentDidClose(t *testing.T) { <-srv.publishChan - if _, ok := srv.documents[uri.URI("file:///test.py")]; !ok { - t.Error("Expected document to be opened") - } + // if _, ok := srv.documents[uri.URI("file:///test.py")]; !ok { + // t.Error("Expected document to be opened") + // } // close document err = client.Notify(lsp.MethodTextDocumentDidClose, lsp.DidCloseTextDocumentParams{ @@ -412,9 +410,9 @@ func TestMethodTextDocumentDidClose(t *testing.T) { // Wait for the document to be closed time.Sleep(100 * time.Millisecond) - if _, ok := srv.documents[uri.URI("file:///test.py")]; ok { - t.Error("Expected document to be closed") - } + // if _, ok := srv.documents[uri.URI("file:///test.py")]; ok { + // t.Error("Expected document to be closed") + // } } func TestMethodTextDocumentDidClose_NoPayload(t *testing.T) {