Merge pull request #6204 from nextcloud/backport/6200/stable28

[stable28] Fix reconnecting websocket polyfill and error propagation during push

src/components/Editor.vue

@@ -562,14 +562,14 @@ export default {
 			this.document = document
 
 			this.syncError = null
-			const editable = !this.readOnly
+			const editable = !this.readOnly && !this.hasConnectionIssue
 			if (this.$editor.isEditable !== editable) {
 				this.$editor.setEditable(editable)
 			}
 		},
 
 		onSync({ steps, document }) {
-			this.hasConnectionIssue = false
+			this.hasConnectionIssue = this.$syncService.backend.fetcher === 0 || !this.$providers[0].wsconnected || this.$syncService.pushError > 0
 			this.$nextTick(() => {
 				this.emit('sync-service:sync')
 			})

src/services/SyncService.js

@@ -95,6 +95,8 @@ class SyncService {
 
 		this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL)
 
+		this.pushError = 0
+
 		return this
 	}
 
@@ -106,10 +108,23 @@ class SyncService {
 		return this.#connection.session.guestName
 	}
 
+	get hasActiveConnection() {
+		return this.#connection && !this.#connection.isClosed
+	}
+
+	get connectionState() {
+		if (!this.#connection || this.version === undefined) {
+			return null
+		}
+		return {
+			...this.#connection.state,
+			version: this.version,
+		}
+	}
+
 	async open({ fileId, initialSession }) {
-		if (this.#connection && !this.#connection.isClosed) {
-			// We're already connected.
-			return
+		if (this.hasActiveConnection) {
+			return this.connectionState
 		}
 		const onChange = ({ sessions }) => {
 			this.sessions = sessions
@@ -124,19 +139,15 @@ class SyncService {
 		if (!this.#connection) {
 			this.off('change', onChange)
 			// Error was already emitted in connect
-			return
+			return null
 		}
 		this.backend = new PollingBackend(this, this.#connection)
 		this.version = this.#connection.docStateVersion
 		this.baseVersionEtag = this.#connection.document.baseVersionEtag
-		this.emit('opened', {
-			...this.#connection.state,
-			version: this.version,
-		})
-		this.emit('loaded', {
-			...this.#connection.state,
-			version: this.version,
-		})
+		this.emit('opened', this.connectionState)
+		this.emit('loaded', this.connectionState)
+
+		return this.connectionState
 	}
 
 	startSync() {
@@ -190,6 +201,7 @@ class SyncService {
 		}
 		return this.#connection.push(data)
 			.then((response) => {
+				this.pushError = 0
 				this.sending = false
 				this.emit('sync', {
 					steps: [],
@@ -199,6 +211,7 @@ class SyncService {
 			}).catch(err => {
 				const { response, code } = err
 				this.sending = false
+				this.pushError++
 				if (!response || code === 'ECONNABORTED') {
 					this.emit('error', { type: ERROR_TYPE.CONNECTION_FAILED, data: {} })
 				}
@@ -215,6 +228,8 @@ class SyncService {
 						this.emit('error', { type: ERROR_TYPE.PUSH_FAILURE, data: {} })
 						OC.Notification.showTemporary('Changes could not be sent yet')
 					}
+				} else {
+					this.emit('error', { type: ERROR_TYPE.PUSH_FAILURE, data: {} })
 				}
 				throw new Error('Failed to apply steps. Retry!', { cause: err })
 			})
@@ -325,7 +340,7 @@ class SyncService {
 		// Make sure to leave no pending requests behind.
 		this.autosave.clear()
 		this.backend?.disconnect()
-		if (!this.#connection || this.#connection.isClosed) {
+		if (!this.hasActiveConnection) {
 			return
 		}
 		return this.#connection.close()

src/services/WebSocketPolyfill.js

@@ -49,10 +49,9 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio
 			logger.debug('WebSocketPolyfill#constructor', { url, fileId, initialSession })
 			this.#registerHandlers({
 				opened: ({ version, session }) => {
-					this.#version = version
 					logger.debug('opened ', { version, session })
+					this.#version = version
 					this.#session = session
-					this.onopen?.()
 				},
 				loaded: ({ version, session, content }) => {
 					logger.debug('loaded ', { version, session })
@@ -70,7 +69,16 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio
 					}
 				},
 			})
-			syncService.open({ fileId, initialSession })
+
+			syncService.open({ fileId, initialSession }).then((data) => {
+				if (syncService.hasActiveConnection) {
+					const { version, session } = data
+					this.#version = version
+					this.#session = session
+
+					this.onopen?.()
+				}
+			})
 		}
 
 		#registerHandlers(handlers) {
@@ -101,7 +109,10 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio
 					...queue.filter(s => !outbox.includes(s)),
 				)
 				return ret
-			}, err => logger.error(err))
+			}, err => {
+				logger.error(`Failed to push the queue with ${queue.length} steps to the server`, err)
+				this.onerror?.(err)
+			})
 		}
 
 		async close() {

src/tests/services/WebsocketPolyfill.spec.js

@@ -3,21 +3,21 @@ import initWebSocketPolyfill from '../../services/WebSocketPolyfill.js'
 describe('Init function', () => {
 
 	it('returns a websocket polyfill class', () => {
-		const syncService = { on: jest.fn(), open: jest.fn() }
+		const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) }
 		const Polyfill = initWebSocketPolyfill(syncService)
 		const websocket = new Polyfill('url')
 		expect(websocket).toBeInstanceOf(Polyfill)
 	})
 
 	it('registers handlers', () => {
-		const syncService = { on: jest.fn(), open: jest.fn() }
+		const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) }
 		const Polyfill = initWebSocketPolyfill(syncService)
 		const websocket = new Polyfill('url')
 		expect(syncService.on).toHaveBeenCalled()
 	})
 
 	it('opens sync service', () => {
-		const syncService = { on: jest.fn(), open: jest.fn() }
+		const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) }
 		const fileId = 123
 		const initialSession = { }
 		const Polyfill = initWebSocketPolyfill(syncService, fileId, initialSession)
@@ -28,7 +28,7 @@ describe('Init function', () => {
 	it('sends steps to sync service', async () => {
 		const syncService = {
 			on: jest.fn(),
-			open: jest.fn(),
+			open: jest.fn(() => Promise.resolve({ version: 123, session: {} })),
 			sendSteps: async getData => getData(),
 		}
 		const queue = [ 'initial' ]
@@ -46,9 +46,10 @@ describe('Init function', () => {
 	})
 
 	it('handles early reject', async () => {
+		jest.spyOn(console, 'error').mockImplementation(() => {})
 		const syncService = {
 			on: jest.fn(),
-			open: jest.fn(),
+			open: jest.fn(() => Promise.resolve({ version: 123, session: {} })),
 			sendSteps: jest.fn().mockRejectedValue('error before reading steps in sync service'),
 		}
 		const queue = [ 'initial' ]
@@ -64,9 +65,10 @@ describe('Init function', () => {
 	})
 
 	it('handles reject after reading data', async () => {
+		jest.spyOn(console, 'error').mockImplementation(() => {})
 		const syncService = {
 			on: jest.fn(),
-			open: jest.fn(),
+			open: jest.fn(() => Promise.resolve({ version: 123, session: {} })),
 			sendSteps: jest.fn().mockImplementation( async getData => {
 				getData()
 				throw 'error when sending in sync service'
@@ -85,9 +87,10 @@ describe('Init function', () => {
 	})
 
 	it('queue survives a close', async () => {
+		jest.spyOn(console, 'error').mockImplementation(() => {})
 		const syncService = {
 			on: jest.fn(),
-			open: jest.fn(),
+			open: jest.fn(() => Promise.resolve({ version: 123, session: {} })),
 			sendSteps: jest.fn().mockImplementation( async getData => {
 				getData()
 				throw 'error when sending in sync service'