From e222fb17833ec7e958e2d88cb6f89a78ba53130c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 6 Jun 2019 16:53:24 +0200 Subject: [PATCH 01/21] enqueue relations and redactions as well as they might need to wait until their target has been sent --- src/scheduler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scheduler.js b/src/scheduler.js index c6e9e4e717c..ceba188a810 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -177,7 +177,7 @@ MatrixScheduler.RETRY_BACKOFF_RATELIMIT = function(event, attempts, err) { * @see module:scheduler~queueAlgorithm */ MatrixScheduler.QUEUE_MESSAGES = function(event) { - if (event.getType() === "m.room.message") { + if (event.getType() === "m.room.message" || !!event.getTargetId()) { // put these events in the 'message' queue. return "message"; } From c58db665ddf4f426af0a9c3690a26149874ab155 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 6 Jun 2019 16:53:49 +0200 Subject: [PATCH 02/21] give the client a chance to run room.updatePendingEvent after sending before the next event is sent. This is needed to update the target id if it was the local id of the event that was just sent. --- src/scheduler.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/scheduler.js b/src/scheduler.js index ceba188a810..64e29d52a83 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -220,7 +220,14 @@ function _processQueue(scheduler, queueName) { ); // fire the process function and if it resolves, resolve the deferred. Else // invoke the retry algorithm. - scheduler._procFn(obj.event).done(function(res) { + + // First wait for a resolved promise, so the resolve handlers for + // the deferred of the previously sent event can run. + // This way enqueued relations/redactions to enqueued events can receive + // the remove id of their target before being sent. + Promise.resolve().then(() => { + return scheduler._procFn(obj.event); + }).then(function(res) { // remove this from the queue _removeNextEvent(scheduler, queueName); debuglog("Queue '%s' sent event %s", queueName, obj.event.getId()); From 6eb229ac1eb70843432f8c6f783537bd486cbd56 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 15:45:00 +0200 Subject: [PATCH 03/21] first look in pending event list for event being redacted in case the redacted event hasn't been sent yet --- src/models/room.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index 1146f73c3d7..693ce980937 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1143,7 +1143,10 @@ Room.prototype.addPendingEvent = function(event, txnId) { if (event.getType() === "m.room.redaction") { const redactId = event.event.redacts; - const redactedEvent = this.getUnfilteredTimelineSet().findEventById(redactId); + let redactedEvent = this._pendingEventList && this._pendingEventList.find(e => e.getId() === redactId); + if (!redactedEvent) { + redactedEvent = this.getUnfilteredTimelineSet().findEventById(redactId); + } if (redactedEvent) { redactedEvent.markLocallyRedacted(event); this.emit("Room.redaction", event, this); From 831aec6488dd4327721423011f9e3ca5fa4174e5 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 16:10:47 +0200 Subject: [PATCH 04/21] emit remote id once received so enqueued relations have it when sent --- src/client.js | 15 +++++++++++---- src/models/event.js | 24 ++++++++++++++++++++++++ src/models/room.js | 2 +- 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/client.js b/src/client.js index 8f9014289c6..2e2411a8e2f 100644 --- a/src/client.js +++ b/src/client.js @@ -1719,6 +1719,9 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, txnId = this.makeTxnId(); } + // we always construct a MatrixEvent when sending because the store and + // scheduler use them. We'll extract the params back out if it turns out + // the client has no scheduler or store. const localEvent = new MatrixEvent(Object.assign(eventObject, { event_id: "~" + roomId + ":" + txnId, user_id: this.credentials.userId, @@ -1726,13 +1729,17 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, origin_server_ts: new Date().getTime(), })); + const room = this.getRoom(roomId); + const targetId = localEvent.getTargetId(); + if (targetId && targetId.startsWith("~")) { + const target = room.getPendingEvents().find(e => e.getId() === targetId); + target.once("Event.localEventIdReplaced", () => { + localEvent.updateTargetId(target.getId()); + }); + } const type = localEvent.getType(); logger.log(`sendEvent of type ${type} in ${roomId} with txnId ${txnId}`); - // we always construct a MatrixEvent when sending because the store and - // scheduler use them. We'll extract the params back out if it turns out - // the client has no scheduler or store. - const room = this.getRoom(roomId); localEvent._txnId = txnId; localEvent.setStatus(EventStatus.SENDING); diff --git a/src/models/event.js b/src/models/event.js index e16d49b4dbc..d0f67a3a3d8 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -801,6 +801,11 @@ utils.extend(module.exports.MatrixEvent.prototype, { this.emit("Event.status", this, status); }, + replaceLocalEventId(eventId) { + this.event.event_id = eventId; + this.emit("Event.localEventIdReplaced", this); + }, + /** * Get whether the event is a relation event, and of a given type if * `relType` is passed in. @@ -876,6 +881,25 @@ utils.extend(module.exports.MatrixEvent.prototype, { return this._replacingEvent; }, + + getTargetId() { + const relation = this.getRelation(); + if (relation) { + return relation.event_id; + } else if (this.getType() === "m.room.redaction") { + return this.event.redacts; + } + }, + + updateTargetId(eventId) { + const relation = this.getRelation(); + if (relation) { + relation.event_id = eventId; + } else if (this.getType() === "m.room.redaction") { + this.event.redacts = eventId; + } + }, + /** * Summarise the event as JSON for debugging. If encrypted, include both the * decrypted and encrypted view of the event. This is named `toJSON` for use diff --git a/src/models/room.js b/src/models/room.js index 693ce980937..3dd45f7df39 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1318,7 +1318,7 @@ Room.prototype.updatePendingEvent = function(event, newStatus, newEventId) { if (newStatus == EventStatus.SENT) { // update the event id - event.event.event_id = newEventId; + event.replaceLocalEventId(newEventId); // if the event was already in the timeline (which will be the case if // opts.pendingEventOrdering==chronological), we need to update the From 7a10d504b2ea4f889ee9e39840f33612057f679a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 16:15:54 +0200 Subject: [PATCH 05/21] emit Relations.redaction synchronously, timeout should not be needed listeners shouldn't care about the original event, as it's removed from the Relations collection already. --- src/models/relations.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/models/relations.js b/src/models/relations.js index aa5c17d3f9e..22ed41669f5 100644 --- a/src/models/relations.js +++ b/src/models/relations.js @@ -242,12 +242,7 @@ export default class Relations extends EventEmitter { redactedEvent.removeListener("Event.beforeRedaction", this._onBeforeRedaction); - // Dispatch a redaction event on this collection. `setTimeout` is used - // to wait until the next event loop iteration by which time the event - // has actually been marked as redacted. - setTimeout(() => { - this.emit("Relations.redaction"); - }, 0); + this.emit("Relations.redaction"); } /** From f1336a5ce71349c95706b7d96e2f3daa3a4a49b6 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 16:56:50 +0200 Subject: [PATCH 06/21] rename target id to related id and add jsdoc comments --- src/client.js | 4 ++-- src/models/event.js | 18 +++++++++++++++--- src/scheduler.js | 2 +- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/client.js b/src/client.js index 2e2411a8e2f..45a3cfded36 100644 --- a/src/client.js +++ b/src/client.js @@ -1730,11 +1730,11 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, })); const room = this.getRoom(roomId); - const targetId = localEvent.getTargetId(); + const targetId = localEvent.getRelatedId(); if (targetId && targetId.startsWith("~")) { const target = room.getPendingEvents().find(e => e.getId() === targetId); target.once("Event.localEventIdReplaced", () => { - localEvent.updateTargetId(target.getId()); + localEvent.updateRelatedId(target.getId()); }); } const type = localEvent.getType(); diff --git a/src/models/event.js b/src/models/event.js index d0f67a3a3d8..61198c8f637 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -881,8 +881,12 @@ utils.extend(module.exports.MatrixEvent.prototype, { return this._replacingEvent; }, - - getTargetId() { + /** + * For relations and redactions, returns the event_id this event is referring to. + * + * @return {string?} + */ + getRelatedId() { const relation = this.getRelation(); if (relation) { return relation.event_id; @@ -891,7 +895,15 @@ utils.extend(module.exports.MatrixEvent.prototype, { } }, - updateTargetId(eventId) { + /** + * Update the related id with a new one. + * + * Used to replace a local id with remote one before sending + * an event with a related id. + * + * @param {string} eventId the new event id + */ + updateRelatedId(eventId) { const relation = this.getRelation(); if (relation) { relation.event_id = eventId; diff --git a/src/scheduler.js b/src/scheduler.js index 64e29d52a83..cda8c21ec02 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -177,7 +177,7 @@ MatrixScheduler.RETRY_BACKOFF_RATELIMIT = function(event, attempts, err) { * @see module:scheduler~queueAlgorithm */ MatrixScheduler.QUEUE_MESSAGES = function(event) { - if (event.getType() === "m.room.message" || !!event.getTargetId()) { + if (event.getType() === "m.room.message" || !!event.getRelatedId()) { // put these events in the 'message' queue. return "message"; } From 3f917b39c9e30113100b6899451976505f2ea794 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 17:05:02 +0200 Subject: [PATCH 07/21] fix lint --- src/models/room.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index 3dd45f7df39..f527d83cb8d 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1143,7 +1143,8 @@ Room.prototype.addPendingEvent = function(event, txnId) { if (event.getType() === "m.room.redaction") { const redactId = event.event.redacts; - let redactedEvent = this._pendingEventList && this._pendingEventList.find(e => e.getId() === redactId); + let redactedEvent = this._pendingEventList && + this._pendingEventList.find(e => e.getId() === redactId); if (!redactedEvent) { redactedEvent = this.getUnfilteredTimelineSet().findEventById(redactId); } From 7d2f7fae45d244a09bd4da46bef22042ed0c59a9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 7 Jun 2019 19:15:18 +0200 Subject: [PATCH 08/21] fix tests --- spec/unit/scheduler.spec.js | 69 ++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/spec/unit/scheduler.spec.js b/spec/unit/scheduler.spec.js index 502edb3f333..500704ec300 100644 --- a/spec/unit/scheduler.spec.js +++ b/spec/unit/scheduler.spec.js @@ -48,7 +48,7 @@ describe("MatrixScheduler", function() { clock.uninstall(); }); - it("should process events in a queue in a FIFO manner", function(done) { + it("should process events in a queue in a FIFO manner", async function() { retryFn = function() { return 0; }; @@ -57,28 +57,30 @@ describe("MatrixScheduler", function() { }; const deferA = Promise.defer(); const deferB = Promise.defer(); - let resolvedA = false; + let yieldedA = false; scheduler.setProcessFunction(function(event) { - if (resolvedA) { + if (yieldedA) { expect(event).toEqual(eventB); return deferB.promise; } else { + yieldedA = true; expect(event).toEqual(eventA); return deferA.promise; } }); - scheduler.queueEvent(eventA); - scheduler.queueEvent(eventB).done(function() { - expect(resolvedA).toBe(true); - done(); - }); - deferA.resolve({}); - resolvedA = true; - deferB.resolve({}); + const abPromise = Promise.all([ + scheduler.queueEvent(eventA), + scheduler.queueEvent(eventB), + ]); + deferB.resolve({b: true}); + deferA.resolve({a: true}); + const [a, b] = await abPromise; + expect(a.a).toEqual(true); + expect(b.b).toEqual(true); }); it("should invoke the retryFn on failure and wait the amount of time specified", - function(done) { + async function() { const waitTimeMs = 1500; const retryDefer = Promise.defer(); retryFn = function() { @@ -97,24 +99,27 @@ describe("MatrixScheduler", function() { return defer.promise; } else if (procCount === 2) { // don't care about this defer - return Promise.defer().promise; + return new Promise(); } expect(procCount).toBeLessThan(3); }); scheduler.queueEvent(eventA); + // as queing doesn't start processing + // synchronously anymore (see commit bbdb5ac) + // wait just long enough before it does + await Promise.resolve(); expect(procCount).toEqual(1); defer.reject({}); - retryDefer.promise.done(function() { - expect(procCount).toEqual(1); - clock.tick(waitTimeMs); - expect(procCount).toEqual(2); - done(); - }); + await retryDefer.promise; + expect(procCount).toEqual(1); + clock.tick(waitTimeMs); + await Promise.resolve(); + expect(procCount).toEqual(2); }); it("should give up if the retryFn on failure returns -1 and try the next event", - function(done) { + async function() { // Queue A & B. // Reject A and return -1 on retry. // Expect B to be tried next and the promise for A to be rejected. @@ -122,8 +127,8 @@ describe("MatrixScheduler", function() { return -1; }; queueFn = function() { - return "yep"; -}; + return "yep"; + }; const deferA = Promise.defer(); const deferB = Promise.defer(); @@ -142,13 +147,18 @@ describe("MatrixScheduler", function() { const globalA = scheduler.queueEvent(eventA); scheduler.queueEvent(eventB); - + // as queing doesn't start processing + // synchronously anymore (see commit bbdb5ac) + // wait just long enough before it does + await Promise.resolve(); expect(procCount).toEqual(1); deferA.reject({}); - globalA.catch(function() { + try { + await globalA; + } catch(err) { + await Promise.resolve(); expect(procCount).toEqual(2); - done(); - }); + } }); it("should treat each queue separately", function(done) { @@ -300,7 +310,12 @@ describe("MatrixScheduler", function() { expect(ev).toEqual(eventA); return defer.promise; }); - expect(procCount).toEqual(1); + // as queing doesn't start processing + // synchronously anymore (see commit bbdb5ac) + // wait just long enough before it does + Promise.resolve().then(() => { + expect(procCount).toEqual(1); + }); }); it("should not call the processFn if there are no queued events", function() { From 624c6f0a6eebea771650c29032af9de67147e7fe Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 11 Jun 2019 18:24:48 +0200 Subject: [PATCH 09/21] get the txnId from the correct place to delete event after remote echo --- src/models/room.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index f527d83cb8d..46ee612c05e 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1215,7 +1215,7 @@ Room.prototype._handleRemoteEcho = function(remoteEvent, localEvent) { const oldStatus = localEvent.status; // no longer pending - delete this._txnToEvent[remoteEvent.transaction_id]; + delete this._txnToEvent[remoteEvent.getUnsigned().transaction_id]; // if it's in the pending list, remove it if (this._pendingEventList) { From 6d9fba8191ae4282ff490ab77ab0d1dbb2dd50f8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 11 Jun 2019 18:25:25 +0200 Subject: [PATCH 10/21] preserve (locally) redacted state after applying remote echo because the RedactionDimensions was trying to redact an event that was already redacted after it's remote echo had come in but it's redaction hadn't synced yet. --- src/models/event.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/models/event.js b/src/models/event.js index 61198c8f637..43036acc9ef 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -776,9 +776,23 @@ utils.extend(module.exports.MatrixEvent.prototype, { * @param {Object} event the object to assign to the `event` property */ handleRemoteEcho: function(event) { + const oldUnsigned = this.getUnsigned(); + const oldId = this.getId(); this.event = event; + // keep being redacted if + // the event was redacted as a local echo + if (oldUnsigned.redacted_because) { + if (!this.event.unsigned) { + this.event.unsigned = {}; + } + this.event.unsigned.redacted_because = oldUnsigned.redacted_because; + } // successfully sent. this.setStatus(null); + if (this.getId() !== oldId) { + // emit the event if it changed + this.emit("Event.localEventIdReplaced", this); + } }, /** From 930de640aca846ddd39b26de113e2ce808c3cecc Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 12 Jun 2019 10:01:52 +0200 Subject: [PATCH 11/21] don't add events from /sync that have been locally redacted it'll cause the reactions counter to go up and down while reactions and redactions come in. In case the local redaction gets cancelled, Room._revertRedactionLocalEcho will add the relation back to the relations collection. --- src/models/event-timeline-set.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index af2d4875919..5b852f5b0ae 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -749,6 +749,10 @@ EventTimelineSet.prototype.aggregateRelations = function(event) { return; } + if (event.isRedacted()) { + return; + } + // If the event is currently encrypted, wait until it has been decrypted. if (event.isBeingDecrypted()) { event.once("Event.decrypted", () => { From 5602b94dcbb5b73478b2f09900b4659598dea34c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 12 Jun 2019 10:22:39 +0200 Subject: [PATCH 12/21] make sure where not re-adding cancelled events when undoing local red. --- src/models/room.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index 46ee612c05e..02d999cea3e 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1356,7 +1356,10 @@ Room.prototype._revertRedactionLocalEcho = function(redactionEvent) { // re-render after undoing redaction this.emit("Room.redactionCancelled", redactionEvent, this); // reapply relation now redaction failed - if (redactedEvent.isRelation()) { + if ( + redactedEvent.isRelation() && + redactedEvent.status !== EventStatus.CANCELLED + ) { this._aggregateNonLiveRelation(redactedEvent); } } From a9f9e2cf352f2d87c1a552f527a70ede6f625bc8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 12 Jun 2019 14:04:52 +0000 Subject: [PATCH 13/21] comment typo Co-Authored-By: J. Ryan Stinnett --- spec/unit/scheduler.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/scheduler.spec.js b/spec/unit/scheduler.spec.js index 500704ec300..3d1fc76dabc 100644 --- a/spec/unit/scheduler.spec.js +++ b/spec/unit/scheduler.spec.js @@ -105,7 +105,7 @@ describe("MatrixScheduler", function() { }); scheduler.queueEvent(eventA); - // as queing doesn't start processing + // as queueing doesn't start processing // synchronously anymore (see commit bbdb5ac) // wait just long enough before it does await Promise.resolve(); From b005b753319a7aa397159ab5c14b05d83c60aa87 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 12 Jun 2019 14:06:43 +0000 Subject: [PATCH 14/21] comment typo Co-Authored-By: J. Ryan Stinnett --- spec/unit/scheduler.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/scheduler.spec.js b/spec/unit/scheduler.spec.js index 3d1fc76dabc..a513f97b955 100644 --- a/spec/unit/scheduler.spec.js +++ b/spec/unit/scheduler.spec.js @@ -310,7 +310,7 @@ describe("MatrixScheduler", function() { expect(ev).toEqual(eventA); return defer.promise; }); - // as queing doesn't start processing + // as queueing doesn't start processing // synchronously anymore (see commit bbdb5ac) // wait just long enough before it does Promise.resolve().then(() => { From 3ed9b003989c6690fcd7b341a5cd8e9f04c05335 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 11:47:01 +0200 Subject: [PATCH 15/21] clarify why we need to listen for remote echo of related event --- src/client.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/client.js b/src/client.js index 45a3cfded36..20b29d13a21 100644 --- a/src/client.js +++ b/src/client.js @@ -1730,6 +1730,11 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, })); const room = this.getRoom(roomId); + + // if this is a relation or redaction of an event + // that hasn't been sent yet (e.g. with a local id starting with a ~) + // then listen for the remote echo of that event so that by the time + // this event does get sent, we have the correct event_id const targetId = localEvent.getRelatedId(); if (targetId && targetId.startsWith("~")) { const target = room.getPendingEvents().find(e => e.getId() === targetId); From 4143a79f7bb68f2a7ab92d8bc0831db148359630 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 11:50:29 +0200 Subject: [PATCH 16/21] rename related id to associated id --- src/client.js | 4 ++-- src/models/event.js | 4 ++-- src/scheduler.js | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/client.js b/src/client.js index 20b29d13a21..1f86301d39f 100644 --- a/src/client.js +++ b/src/client.js @@ -1735,11 +1735,11 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, // that hasn't been sent yet (e.g. with a local id starting with a ~) // then listen for the remote echo of that event so that by the time // this event does get sent, we have the correct event_id - const targetId = localEvent.getRelatedId(); + const targetId = localEvent.getAssociatedId(); if (targetId && targetId.startsWith("~")) { const target = room.getPendingEvents().find(e => e.getId() === targetId); target.once("Event.localEventIdReplaced", () => { - localEvent.updateRelatedId(target.getId()); + localEvent.updateAssociatedId(target.getId()); }); } const type = localEvent.getType(); diff --git a/src/models/event.js b/src/models/event.js index 43036acc9ef..a1784d6b479 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -900,7 +900,7 @@ utils.extend(module.exports.MatrixEvent.prototype, { * * @return {string?} */ - getRelatedId() { + getAssociatedId() { const relation = this.getRelation(); if (relation) { return relation.event_id; @@ -917,7 +917,7 @@ utils.extend(module.exports.MatrixEvent.prototype, { * * @param {string} eventId the new event id */ - updateRelatedId(eventId) { + updateAssociatedId(eventId) { const relation = this.getRelation(); if (relation) { relation.event_id = eventId; diff --git a/src/scheduler.js b/src/scheduler.js index cda8c21ec02..b3461bebd75 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -177,7 +177,8 @@ MatrixScheduler.RETRY_BACKOFF_RATELIMIT = function(event, attempts, err) { * @see module:scheduler~queueAlgorithm */ MatrixScheduler.QUEUE_MESSAGES = function(event) { - if (event.getType() === "m.room.message" || !!event.getRelatedId()) { + // enqueue messages or events that associate with another event (redactions and relations) + if (event.getType() === "m.room.message" || !!event.getAssociatedId()) { // put these events in the 'message' queue. return "message"; } From 4462f4b90e3dfacb29395c0da12a227e9d5295f9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 11:57:39 +0200 Subject: [PATCH 17/21] add isRedaction helper on Event --- src/client.js | 2 +- src/models/event.js | 13 +++++++++++-- src/models/room.js | 8 ++++---- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/client.js b/src/client.js index 1f86301d39f..239dc3b1af8 100644 --- a/src/client.js +++ b/src/client.js @@ -1898,7 +1898,7 @@ function _sendEventHttpRequest(client, event) { pathTemplate = "/rooms/$roomId/state/$eventType/$stateKey"; } path = utils.encodeUri(pathTemplate, pathParams); - } else if (event.getType() === "m.room.redaction") { + } else if (event.isRedaction()) { const pathTemplate = `/rooms/$roomId/redact/$redactsEventId/$txnId`; path = utils.encodeUri(pathTemplate, Object.assign({ $redactsEventId: event.event.redacts, diff --git a/src/models/event.js b/src/models/event.js index a1784d6b479..abb6f8a2422 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -753,6 +753,15 @@ utils.extend(module.exports.MatrixEvent.prototype, { return Boolean(this.getUnsigned().redacted_because); }, + /** + * Check if this event is a redaction of another event + * + * @return {boolean} True if this event is a redaction + */ + isRedaction: function() { + return this.getType() === "m.room.redaction"; + }, + /** * Get the push actions, if known, for this event * @@ -904,7 +913,7 @@ utils.extend(module.exports.MatrixEvent.prototype, { const relation = this.getRelation(); if (relation) { return relation.event_id; - } else if (this.getType() === "m.room.redaction") { + } else if (this.isRedaction()) { return this.event.redacts; } }, @@ -921,7 +930,7 @@ utils.extend(module.exports.MatrixEvent.prototype, { const relation = this.getRelation(); if (relation) { relation.event_id = eventId; - } else if (this.getType() === "m.room.redaction") { + } else if (this.isRedaction()) { this.event.redacts = eventId; } }, diff --git a/src/models/room.js b/src/models/room.js index 02d999cea3e..46eda57ec83 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1032,7 +1032,7 @@ Room.prototype.removeFilteredTimelineSet = function(filter) { * @private */ Room.prototype._addLiveEvent = function(event, duplicateStrategy) { - if (event.getType() === "m.room.redaction") { + if (event.isRedaction()) { const redactId = event.event.redacts; // if we know about this event, redact its contents now. @@ -1141,7 +1141,7 @@ Room.prototype.addPendingEvent = function(event, txnId) { this._aggregateNonLiveRelation(event); } - if (event.getType() === "m.room.redaction") { + if (event.isRedaction()) { const redactId = event.event.redacts; let redactedEvent = this._pendingEventList && this._pendingEventList.find(e => e.getId() === redactId); @@ -1333,7 +1333,7 @@ Room.prototype.updatePendingEvent = function(event, newStatus, newEventId) { const idx = this._pendingEventList.findIndex(ev => ev.getId() === oldEventId); if (idx !== -1) { const [removedEvent] = this._pendingEventList.splice(idx, 1); - if (removedEvent.getType() === "m.room.redaction") { + if (removedEvent.isRedaction()) { this._revertRedactionLocalEcho(removedEvent); } } @@ -1442,7 +1442,7 @@ Room.prototype.removeEvent = function(eventId) { for (let i = 0; i < this._timelineSets.length; i++) { const removed = this._timelineSets[i].removeEvent(eventId); if (removed) { - if (removed.getType() === "m.room.redaction") { + if (removed.isRedaction()) { this._revertRedactionLocalEcho(removed); } removedAny = true; From 811a98ad1953f1ffb6b8b00d5310bf4dc5782f21 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 12:03:20 +0200 Subject: [PATCH 18/21] whitespace, newlines --- spec/unit/scheduler.spec.js | 9 +++------ src/client.js | 1 + 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/unit/scheduler.spec.js b/spec/unit/scheduler.spec.js index a513f97b955..b518e832377 100644 --- a/spec/unit/scheduler.spec.js +++ b/spec/unit/scheduler.spec.js @@ -105,8 +105,7 @@ describe("MatrixScheduler", function() { }); scheduler.queueEvent(eventA); - // as queueing doesn't start processing - // synchronously anymore (see commit bbdb5ac) + // as queueing doesn't start processing synchronously anymore (see commit bbdb5ac) // wait just long enough before it does await Promise.resolve(); expect(procCount).toEqual(1); @@ -147,8 +146,7 @@ describe("MatrixScheduler", function() { const globalA = scheduler.queueEvent(eventA); scheduler.queueEvent(eventB); - // as queing doesn't start processing - // synchronously anymore (see commit bbdb5ac) + // as queueing doesn't start processing synchronously anymore (see commit bbdb5ac) // wait just long enough before it does await Promise.resolve(); expect(procCount).toEqual(1); @@ -310,8 +308,7 @@ describe("MatrixScheduler", function() { expect(ev).toEqual(eventA); return defer.promise; }); - // as queueing doesn't start processing - // synchronously anymore (see commit bbdb5ac) + // as queueing doesn't start processing synchronously anymore (see commit bbdb5ac) // wait just long enough before it does Promise.resolve().then(() => { expect(procCount).toEqual(1); diff --git a/src/client.js b/src/client.js index 239dc3b1af8..4c146b9d0e0 100644 --- a/src/client.js +++ b/src/client.js @@ -1742,6 +1742,7 @@ MatrixClient.prototype._sendCompleteEvent = function(roomId, eventObject, txnId, localEvent.updateAssociatedId(target.getId()); }); } + const type = localEvent.getType(); logger.log(`sendEvent of type ${type} in ${roomId} with txnId ${txnId}`); From 3488fbe64cb3e66a1c536fa0ead7cf7ec86a34e9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 12:03:35 +0200 Subject: [PATCH 19/21] expand comment why need to preserve redaction local echo on remote echo --- src/models/event.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/models/event.js b/src/models/event.js index abb6f8a2422..560ca110325 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -788,8 +788,11 @@ utils.extend(module.exports.MatrixEvent.prototype, { const oldUnsigned = this.getUnsigned(); const oldId = this.getId(); this.event = event; - // keep being redacted if - // the event was redacted as a local echo + // if this event was redacted before it was sent, it's locally marked as redacted. + // At this point, we've received the remote echo for the event, but not yet for + // the redaction that we are sending ourselves. Preserve the locally redacted + // state by copying over redacted_because so we don't get a flash of + // redacted, not-redacted, redacted as remote echos come in if (oldUnsigned.redacted_because) { if (!this.event.unsigned) { this.event.unsigned = {}; From 2a0c85c7723bd57f5ad8bc9b4ea0c8efaeb538f8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 12:06:54 +0200 Subject: [PATCH 20/21] add hasAssociation helper --- src/models/event.js | 9 +++++++++ src/scheduler.js | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/models/event.js b/src/models/event.js index 560ca110325..50adc34a8d8 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -921,6 +921,15 @@ utils.extend(module.exports.MatrixEvent.prototype, { } }, + /** + * Checks if this event is associated with another event. See `getAssociatedId`. + * + * @return {bool} + */ + hasAssocation() { + return !!this.getAssociatedId(); + }, + /** * Update the related id with a new one. * diff --git a/src/scheduler.js b/src/scheduler.js index b3461bebd75..a799597b9de 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -178,7 +178,7 @@ MatrixScheduler.RETRY_BACKOFF_RATELIMIT = function(event, attempts, err) { */ MatrixScheduler.QUEUE_MESSAGES = function(event) { // enqueue messages or events that associate with another event (redactions and relations) - if (event.getType() === "m.room.message" || !!event.getAssociatedId()) { + if (event.getType() === "m.room.message" || event.hasAssocation()) { // put these events in the 'message' queue. return "message"; } From 6059df1b678db623702d7b4f0dd6736f7e4c7c9a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 13 Jun 2019 12:12:02 +0200 Subject: [PATCH 21/21] move CANCELLED check deeper into aggregation path --- src/models/event-timeline-set.js | 3 ++- src/models/room.js | 5 +---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 5b852f5b0ae..8f2fad30e73 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -20,6 +20,7 @@ limitations under the License. const EventEmitter = require("events").EventEmitter; const utils = require("../utils"); const EventTimeline = require("./event-timeline"); +import {EventStatus} from "./event"; import logger from '../../src/logger'; import Relations from './relations'; @@ -749,7 +750,7 @@ EventTimelineSet.prototype.aggregateRelations = function(event) { return; } - if (event.isRedacted()) { + if (event.isRedacted() || event.status === EventStatus.CANCELLED) { return; } diff --git a/src/models/room.js b/src/models/room.js index 46eda57ec83..a77aaa24f80 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -1356,10 +1356,7 @@ Room.prototype._revertRedactionLocalEcho = function(redactionEvent) { // re-render after undoing redaction this.emit("Room.redactionCancelled", redactionEvent, this); // reapply relation now redaction failed - if ( - redactedEvent.isRelation() && - redactedEvent.status !== EventStatus.CANCELLED - ) { + if (redactedEvent.isRelation()) { this._aggregateNonLiveRelation(redactedEvent); } }