fix: Potential onCreateDocument race condition

packages/server/src/Hocuspocus.ts

@@ -226,45 +226,40 @@ export class Hocuspocus {
    * @private
    */
   private async createDocument(documentName: string, request: IncomingMessage, socketId: string, context?: any): Promise<Document> {
-    return new Promise(resolve => {
-      if (this.documents.has(documentName)) {
-        const document = this.documents.get(documentName)
-        return resolve(document)
-      }
-
-      const document = new Document(documentName)
+    if (this.documents.has(documentName)) {
+      const document = this.documents.get(documentName)
+      return document
+    }
 
-      document.onUpdate((document, connection, update) => {
-        this.handleDocumentUpdate(document, connection, update, request, connection?.socketId)
-      })
+    const document = new Document(documentName)
+    this.documents.set(documentName, document)
 
-      const hookPayload = {
-        context,
-        document,
-        documentName,
-        socketId,
-        requestHeaders: request.headers,
-        requestParameters: Hocuspocus.getParameters(request),
-      }
+    const hookPayload = {
+      context,
+      document,
+      documentName,
+      socketId,
+      requestHeaders: request.headers,
+      requestParameters: Hocuspocus.getParameters(request),
+    }
 
-      this.hooks('onCreateDocument', hookPayload, (loadedDocument: Doc | undefined) => {
+    await this.hooks('onCreateDocument', hookPayload, (loadedDocument: Doc | undefined) => {
       // if a hook returns a Y-Doc, encode the document state as update
       // and apply it to the newly created document
       // Note: instanceof doesn't work, because Doc !== Doc for some reason I don't understand
-        if (
-          loadedDocument?.constructor.name === 'Document'
+      if (
+        loadedDocument?.constructor.name === 'Document'
         || loadedDocument?.constructor.name === 'Doc'
-        ) {
-          applyUpdate(document, encodeStateAsUpdate(loadedDocument))
-        }
-      }).then(() => {
-        resolve(document)
-      }).catch(e => {
-        throw e
-      })
+      ) {
+        applyUpdate(document, encodeStateAsUpdate(loadedDocument))
+      }
+    })
 
-      this.documents.set(documentName, document)
+    document.onUpdate((document: Document, connection: Connection, update: Uint8Array) => {
+      this.handleDocumentUpdate(document, connection, update, request, connection?.socketId)
     })
+
+    return document
   }
 
   /**

tests/server/onChange.js

@@ -41,4 +41,34 @@ context('server/onChange', () => {
       ydoc.getArray('foo').insert(0, ['bar'])
     })
   })
+
+  it('onChange callback is not called after onCreateDocument', done => {
+    let triggered = false
+
+    Server.configure({
+      port: 4000,
+      async onChange(data) {
+        triggered = true
+      },
+      async onCreateDocument({ document }) {
+        document.getArray('foo').insert(0, ['bar'])
+        return document
+      },
+    }).listen()
+
+    client = new HocuspocusProvider({
+      url: 'ws://127.0.0.1:4000',
+      name: 'hocuspocus-test',
+      document: ydoc,
+      WebSocketPolyfill: WebSocket,
+    })
+
+    client.on('synced', () => {
+      if (triggered) {
+        throw new Error('onChange should not be called unless client updates')
+      }
+
+      done()
+    })
+  })
 })

tests/server/onCreateDocument.js

@@ -12,10 +12,14 @@ context('server/onCreateDocument', () => {
   before(() => {
     Server.configure({
       port: 4000,
-      async onCreateDocument({ document }) {
-        document.getArray('foo').insert(0, ['bar'])
-
-        return document
+      onCreateDocument({ document }) {
+        // delay more accurately simulate a database fetch
+        return new Promise(resolve => {
+          setTimeout(() => {
+            document.getArray('foo').insert(0, ['bar'])
+            resolve(document)
+          }, 250)
+        })
       },
     }).listen()
   })
@@ -42,4 +46,27 @@ context('server/onCreateDocument', () => {
       done()
     })
   })
+
+  it('multiple simultanous connections do not create multiple documents', done => {
+    client = new HocuspocusProvider({
+      url: 'ws://127.0.0.1:4000',
+      name: 'hocuspocus-test',
+      document: ydoc,
+      WebSocketPolyfill: WebSocket,
+    })
+
+    const client2 = new HocuspocusProvider({
+      url: 'ws://127.0.0.1:4000',
+      name: 'hocuspocus-test',
+      document: new Y.Doc(),
+      WebSocketPolyfill: WebSocket,
+    })
+
+    client.on('synced', () => {
+      assert.strictEqual(Server.documents.size, 1)
+
+      client2.destroy()
+      done()
+    })
+  })
 })