From 59bbbdc71314aa91755e5ee0482afa1f34471ab7 Mon Sep 17 00:00:00 2001 From: nezuo Date: Mon, 23 Oct 2023 17:11:55 -0700 Subject: [PATCH 1/2] Add beforeClose --- src/Document.lua | 48 ++++++++++++++++++++++++++--- src/Document.test.lua | 72 +++++++++++++++++++++++++++++++++++++++++++ src/init.lua | 1 + wally.lock | 6 ++-- wally.toml | 2 +- 5 files changed, 120 insertions(+), 9 deletions(-) diff --git a/src/Document.lua b/src/Document.lua index 7f4e902..c56a567 100644 --- a/src/Document.lua +++ b/src/Document.lua @@ -15,6 +15,7 @@ function Document.new(collection, key, validate, lockId, data) lockId = lockId, data = data, closed = false, + callingBeforeClose = false, }, Document) end @@ -55,7 +56,7 @@ end @return Promise<()> ]=] function Document:save() - assert(not self.closed, "Cannot save a closed document") + assert(not self.closed and not self.callingBeforeClose, "Cannot save a closed document") return self.collection.data:save(self.collection.dataStore, self.key, function(value) if value.lockId ~= self.lockId then @@ -79,16 +80,37 @@ end Throws an error if the document was closed. ::: + :::warning + Throws an error or yields if the beforeClose callback does. + ::: + @return Promise<()> ]=] function Document:close() - assert(not self.closed, "Cannot close a closed document") + assert(not self.closed and not self.callingBeforeClose, "Cannot close a closed document") + + local function close() + self.closed = true + + self.collection.openDocuments[self.key] = nil - self.closed = true + self.collection.autoSave:removeDocument(self) + end + + if self.beforeCloseCallback ~= nil then + self.callingBeforeClose = true + + local ok, err = pcall(self.beforeCloseCallback) + + if not ok then + close() + error(`beforeClose callback threw error: {tostring(err)}`) + end - self.collection.openDocuments[self.key] = nil + self.callingBeforeClose = false + end - self.collection.autoSave:removeDocument(self) + close() return self.collection.data:save(self.collection.dataStore, self.key, function(value) if value.lockId ~= self.lockId then @@ -105,4 +127,20 @@ function Document:close() end) end +--[=[ + Sets a callback that is run inside `document:close` before it saves. The document can be read and written to in the + callback. + + :::warning + Throws an error if it was called previously. + ::: + + @param callback () -> () +]=] +function Document:beforeClose(callback) + assert(self.beforeCloseCallback == nil, "Document:beforeClose can only be called once") + + self.beforeCloseCallback = callback +end + return Document diff --git a/src/Document.test.lua b/src/Document.test.lua index 6cde4a7..a6ecfbf 100644 --- a/src/Document.test.lua +++ b/src/Document.test.lua @@ -10,6 +10,7 @@ local DEFAULT_OPTIONS = { } return function(x) + local assertEqual = x.assertEqual local shouldThrow = x.shouldThrow x.test("it should not merge close into save when save is running", function(context) @@ -200,4 +201,75 @@ return function(x) promise:expect() end) + + x.nested("Document:beforeClose", function() + x.test("throws when setting twice", function(context) + local document = context.lapis.createCollection("collection", DEFAULT_OPTIONS):load("document"):expect() + + document:beforeClose(function() end) + + shouldThrow(function() + document:beforeClose(function() end) + end, "Document:beforeClose can only be called once") + end) + + x.test("throws when calling close in callback", function(context) + local document = context.lapis.createCollection("collection", DEFAULT_OPTIONS):load("document"):expect() + + document:beforeClose(function() + document:close() + end) + + shouldThrow(function() + document:close() + end, "beforeClose callback threw error") + end) + + x.test("throws when calling save in callback", function(context) + local document = context.lapis.createCollection("collection", DEFAULT_OPTIONS):load("document"):expect() + + document:beforeClose(function() + document:save() + end) + + shouldThrow(function() + document:close() + end, "beforeClose callback threw error") + end) + + x.test("closes document even if document error", function(context) + local collection = context.lapis.createCollection("collection", DEFAULT_OPTIONS) + + local promise = collection:load("document") + local document = promise:expect() + + document:beforeClose(function() + error("error") + end) + + shouldThrow(function() + document:close() + end) + + assert(collection:load("document") ~= promise, "collection:load should return a new promise") + + shouldThrow(function() + document:write({ foo = "baz" }) + end, "Cannot write to a closed document") + end) + + x.test("saves new data", function(context) + local document = context.lapis.createCollection("collection", DEFAULT_OPTIONS):load("document"):expect() + + document:beforeClose(function() + document:read() -- This checks that read doesn't error in the callback. + + document:write({ foo = "new" }) + end) + + document:close():expect() + + assertEqual(context.read("collection", "document").data.foo, "new") + end) + end) end diff --git a/src/init.lua b/src/init.lua index be73f57..845b1d6 100644 --- a/src/init.lua +++ b/src/init.lua @@ -33,6 +33,7 @@ export type Document = { write: (self: Document, T) -> (), save: (self: Document) -> PromiseTypes.TypedPromise<()>, close: (self: Document) -> PromiseTypes.TypedPromise<()>, + beforeClose: (self: Document, callback: () -> ()) -> (), } --[=[ diff --git a/wally.lock b/wally.lock index f109279..92ee559 100644 --- a/wally.lock +++ b/wally.lock @@ -14,10 +14,10 @@ dependencies = [] [[package]] name = "nezuo/lapis" -version = "0.2.4" -dependencies = [["Promise", "evaera/promise@4.0.0"], ["DataStoreServiceMock", "nezuo/data-store-service-mock@0.3.0"], ["Midori", "nezuo/midori@0.1.2"]] +version = "0.2.5" +dependencies = [["Promise", "evaera/promise@4.0.0"], ["DataStoreServiceMock", "nezuo/data-store-service-mock@0.3.0"], ["Midori", "nezuo/midori@0.1.3"]] [[package]] name = "nezuo/midori" -version = "0.1.2" +version = "0.1.3" dependencies = [] diff --git a/wally.toml b/wally.toml index 1b80dfa..23a108d 100644 --- a/wally.toml +++ b/wally.toml @@ -10,5 +10,5 @@ realm = "server" Promise = "evaera/promise@4.0.0" [dev-dependencies] -Midori = "nezuo/midori@0.1.2" +Midori = "nezuo/midori@0.1.3" DataStoreServiceMock = "nezuo/data-store-service-mock@0.3.0" From c67253a0e3e89a89bd95c0304c4237c5b7634b7e Mon Sep 17 00:00:00 2001 From: nezuo Date: Mon, 23 Oct 2023 18:01:57 -0700 Subject: [PATCH 2/2] Call beforeCloseCallback in a promise --- src/Document.lua | 56 +++++++++++++++++++++++-------------------- src/Document.test.lua | 22 +++++++++++------ 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/src/Document.lua b/src/Document.lua index c56a567..0b6cd3a 100644 --- a/src/Document.lua +++ b/src/Document.lua @@ -1,5 +1,6 @@ local Compression = require(script.Parent.Compression) local freezeDeep = require(script.Parent.freezeDeep) +local Promise = require(script.Parent.Parent.Promise) --[=[ @class Document @@ -81,7 +82,7 @@ end ::: :::warning - Throws an error or yields if the beforeClose callback does. + If the beforeClose callback errors, the returned promise will reject and the data will not be saved. ::: @return Promise<()> @@ -89,42 +90,45 @@ end function Document:close() assert(not self.closed and not self.callingBeforeClose, "Cannot close a closed document") - local function close() - self.closed = true - - self.collection.openDocuments[self.key] = nil - - self.collection.autoSave:removeDocument(self) - end + local promise = Promise.resolve() if self.beforeCloseCallback ~= nil then self.callingBeforeClose = true - local ok, err = pcall(self.beforeCloseCallback) - - if not ok then - close() - error(`beforeClose callback threw error: {tostring(err)}`) - end + promise = Promise.new(function(resolve, reject) + local ok, err = pcall(self.beforeCloseCallback) - self.callingBeforeClose = false + if not ok then + reject(`beforeClose callback threw error: {tostring(err)}`) + else + resolve() + end + end) end - close() + return promise + :finally(function() + self.closed = true - return self.collection.data:save(self.collection.dataStore, self.key, function(value) - if value.lockId ~= self.lockId then - return "fail", "The session lock was stolen" - end + self.collection.openDocuments[self.key] = nil - local scheme, compressed = Compression.compress(self.data) + self.collection.autoSave:removeDocument(self) + end) + :andThen(function() + return self.collection.data:save(self.collection.dataStore, self.key, function(value) + if value.lockId ~= self.lockId then + return "fail", "The session lock was stolen" + end - value.compressionScheme = scheme - value.data = compressed - value.lockId = nil + local scheme, compressed = Compression.compress(self.data) - return "succeed", value - end) + value.compressionScheme = scheme + value.data = compressed + value.lockId = nil + + return "succeed", value + end) + end) end --[=[ diff --git a/src/Document.test.lua b/src/Document.test.lua index a6ecfbf..017ea1f 100644 --- a/src/Document.test.lua +++ b/src/Document.test.lua @@ -44,11 +44,14 @@ return function(x) local pendingSave = document:save() local pendingClose = document:close() -- This should override the pending save. + context.dataStoreService.budget.budgets[Enum.DataStoreRequestType.UpdateAsync] = 1 + context.dataStoreService.yield:stopYield() - assert(pendingSave == pendingClose, "save and close didn't merge") + local values = Promise.all({ ongoingSave, pendingSave }):expect() - local values = Promise.all({ ongoingSave, pendingSave, pendingClose }):expect() + -- If the save and close don't merge, this will throw because there isn't enough budget to close. + pendingClose:now("save and close didn't merge"):expect() -- save and close should never resolve with a value. -- It's checked in this test to make sure it works with save merging. @@ -221,7 +224,7 @@ return function(x) end) shouldThrow(function() - document:close() + document:close():expect() end, "beforeClose callback threw error") end) @@ -233,11 +236,11 @@ return function(x) end) shouldThrow(function() - document:close() + document:close():expect() end, "beforeClose callback threw error") end) - x.test("closes document even if document error", function(context) + x.test("closes document even if beforeClose errors", function(context) local collection = context.lapis.createCollection("collection", DEFAULT_OPTIONS) local promise = collection:load("document") @@ -248,14 +251,19 @@ return function(x) end) shouldThrow(function() - document:close() + document:close():expect() end) - assert(collection:load("document") ~= promise, "collection:load should return a new promise") + local secondPromise = collection:load("document") + + assert(secondPromise ~= promise, "collection:load should return a new promise") shouldThrow(function() document:write({ foo = "baz" }) end, "Cannot write to a closed document") + + -- Ignore the could not acquire lock error. + secondPromise:catch(function() end) end) x.test("saves new data", function(context)