fix LSP thread safety

4 files changed, 70 insertions, 43 deletions

diff --git a/lsp-write.c b/lsp-write.c
index 93bdd26..f633c2e 100644
--- a/lsp-write.c
+++ b/lsp-write.c
@@ -32,14 +32,6 @@ static const char *lsp_language_id(Language lang) {
 	return "text";
 }
 
-static const char *lsp_document_path(LSP *lsp, LSPDocumentID document) {
-	if (document >= arr_len(lsp->document_data)) {
-		assert(0);
-		return "";
-	}
-	return lsp->document_data[document].path;
-}
-
 typedef struct {
 	LSP *lsp;
 	StrBuilder builder;
@@ -425,15 +417,15 @@ static void write_request(LSP *lsp, LSPRequest *request) {
 	} break;
 	case LSP_REQUEST_DID_CHANGE: {
 		LSPRequestDidChange *change = &request->data.change;
-		if (change->document >= arr_len(lsp->document_data)) {
-			assert(0);
-			break;
-		}
-		LSPDocumentData *document = &lsp->document_data[change->document];
-		++document->version_number;
+		SDL_LockMutex(lsp->document_mutex);
+			assert(change->document < arr_len(lsp->document_data));
+			LSPDocumentData *document = &lsp->document_data[change->document];
+			u32 version = ++document->version_number;
+		SDL_UnlockMutex(lsp->document_mutex);
+		
 		write_key_obj_start(o, "params");
 			write_key_obj_start(o, "textDocument");
-				write_key_number(o, "version", (double)document->version_number);
+				write_key_number(o, "version", version);
 				write_key_file_uri(o, "uri", change->document);
 			write_obj_end(o);
 			write_key_arr_start(o, "contentChanges");
diff --git a/lsp.c b/lsp.c
index 025cc10..a490aad 100644
--- a/lsp.c
+++ b/lsp.c
@@ -309,15 +309,29 @@ static int lsp_communication_thread(void *data) {
 }
 
 u32 lsp_document_id(LSP *lsp, const char *path) {
-	u32 *value = str_hash_table_get(&lsp->document_ids, path);
-	if (!value) {
-		u32 id = arr_len(lsp->document_data);
-		value = str_hash_table_insert(&lsp->document_ids, path);
-		*value = id;
-		LSPDocumentData *data = arr_addp(lsp->document_data);
-		data->path = str_dup(path);
-	}
-	return *value;
+	SDL_LockMutex(lsp->document_mutex);
+		u32 *value = str_hash_table_get(&lsp->document_ids, path);
+		if (!value) {
+			u32 id = arr_len(lsp->document_data);
+			value = str_hash_table_insert(&lsp->document_ids, path);
+			*value = id;
+			LSPDocumentData *data = arr_addp(lsp->document_data);
+			data->path = str_dup(path);
+		}
+		u32 id = *value;
+	SDL_UnlockMutex(lsp->document_mutex);
+	return id;
+}
+
+const char *lsp_document_path(LSP *lsp, LSPDocumentID document) {
+	SDL_LockMutex(lsp->document_mutex);
+		if (document >= arr_len(lsp->document_data)) {
+			assert(0);
+			return "";
+		}
+		const char *path = lsp->document_data[document].path;
+	SDL_UnlockMutex(lsp->document_mutex);
+	return path;
 }
 
 LSP *lsp_create(const char *root_dir, Language language, const char *analyzer_command) {
diff --git a/lsp.h b/lsp.h
index c3770bb..7fce761 100644
--- a/lsp.h
+++ b/lsp.h
@@ -271,35 +271,56 @@ typedef struct {
 } LSPCapabilities;
 
 typedef struct LSP {
+	// thread safety is important here!
+	// every member should either be indented to indicate which mutex controls it,
+	// or have a comment explaining why it doesn't need one
+
+	// A unique ID number for this LSP.
+	// thread-safety: only set once in lsp_create.
 	LSPID id;
+	
+	// The server process
+	// thread-safety: created in lsp_create, then only accessed by the communication thread
 	Process process;
+	
+	// Which ID number the next request will get
+	// thread-safety: only accessed in communication thread
 	u32 request_id;
-	// for our purposes, folders are "documents"
-	// the spec kinda does this too: WorkspaceFolder has a `uri: DocumentUri` member.
-	StrHashTable document_ids; // values are u32. they are indices into document_data.
-	// this is a dynamic array which just keeps growing.
-	// but the user isn't gonna open millions of files so it's fine.
-	LSPDocumentData *document_data;
+	
+	SDL_mutex *document_mutex;
+		// for our purposes, folders are "documents"
+		// the spec kinda does this too: WorkspaceFolder has a `uri: DocumentUri` member.
+		StrHashTable document_ids; // values are u32. they are indices into document_data.
+		// this is a dynamic array which just keeps growing.
+		// but the user isn't gonna open millions of files so it's fine.
+		LSPDocumentData *document_data;
 	SDL_mutex *messages_mutex;
-	LSPMessage *messages_server2client;
-	LSPMessage *messages_client2server;
-	// we keep track of client-to-server requests
-	// so that we can process responses.
-	// this also lets us re-send requests if that's ever necessary.
-	LSPRequest *requests_sent;
+		LSPMessage *messages_server2client;
+		LSPMessage *messages_client2server;
+		// we keep track of client-to-server requests
+		// so that we can process responses.
+		// this also lets us re-send requests if that's ever necessary.
+		LSPRequest *requests_sent;
 	// has the response to the initialize request been sent?
-	//    we access this both in the main thread and in the LSP communication thread.
+	// thread-safety: this is atomic. it starts out false, and only gets set to true once
+	//                (when the initialize response is received)
 	_Atomic bool initialized;
+	// thread-safety: only set once in lsp_create.
+	Language language;
 	SDL_Thread *communication_thread;
 	SDL_sem *quit_sem;
+	// thread-safety: only accessed in communication thread
 	char *received_data; // dynamic array
+	// thread-safety: in the communication thread, we fill this in, then set `initialized = true`.
+	//                after that, this never changes.
+	//                never accessed in main thread before `initialized = true`.
 	LSPCapabilities capabilities;
+	// thread-safety: same as `capabilities`
 	char32_t *trigger_chars; // dynamic array of "trigger characters"
-	SDL_mutex *error_mutex;
-	Language language;
 	SDL_mutex *workspace_folders_mutex;
-	LSPDocumentID *workspace_folders; // dynamic array of root directories of LSP workspace folders
-	char error[256];
+		LSPDocumentID *workspace_folders; // dynamic array of root directories of LSP workspace folders
+	SDL_mutex *error_mutex;
+		char error[256];
 } LSP;
 
 // @TODO: function declarations
@@ -311,6 +332,8 @@ typedef struct LSP {
 bool lsp_get_error(LSP *lsp, char *error, size_t error_size, bool clear);
 void lsp_message_free(LSPMessage *message);
 u32 lsp_document_id(LSP *lsp, const char *path);
+// returned pointer lives exactly as long as lsp.
+const char *lsp_document_path(LSP *lsp, LSPDocumentID id);
 // don't free the contents of this request! let me handle it!
 void lsp_send_request(LSP *lsp, LSPRequest *request);
 // don't free the contents of this response! let me handle it!
diff --git a/main.c b/main.c
index 350e717..a0abf0a 100644
--- a/main.c
+++ b/main.c
@@ -1,7 +1,5 @@
 /*
 @TODO:
-- lsp_document_id / lsp_document_path thread-safety
-- double check thread safety of other things
 - ignore telemetry/event
 - https://github.com/typescript-language-server/typescript-language-server
    - NOTE: This supports javascript.