MaxListenersExceededWarning on threads; many redundant fetches of thread root event

[davidisaaclee]

Using an account that has some threads, I get the following warning around the first sync:

MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 Thread.update listeners added. Use emitter.setMaxListeners() to increase limit

Stack trace

MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 Thread.update listeners added. Use emitter.setMaxListeners() to increase limit
    _addListener events.js:211
    addListener events.js:227
    on typed-event-emitter.ts:133
    reEmit ReEmitter.ts:57
    reEmit ReEmitter.ts:84
    setThread event.ts:1618
    mapper event-mapper.ts:74
    fetchRootEvent thread.ts:148
    updateThreadMetadata thread.ts:482
    addEvents thread.ts:291
    createThread room.ts:2277
    addThreadedEvents room.ts:2183
    addLiveEvents room.ts:2843
    addLiveEvents room.ts:2842
    injectRoomEvents sync.ts:1779
    processSyncResponse sync.ts:1437
    promiseMapSeries utils.ts:425
    processSyncResponse sync.ts:1293
    doSync sync.ts:915
    sync sync.ts:798
    startClient client.ts:1513
    initializeMatrixConnection App.tsx:39
    App App.tsx:46

I haven't encountered any user-impacting issues around this, but it sounds like this could cause MatrixEvents to fail to emit Thread.update to certain listeners. It looks like this is simply a warning and does not prevent listeners from being added: https://github.com/browserify/events/blob/48e3d18659caf72d94d319871106f089bb40002d/events.js#L211 . This is still noise that could mask a real, easily-identifiable memory leak.

More importantly, I believe this is caused by a bug in Thread#fetchRootEvent where the remote root event is fetched on every call to updateThreadMetadata (e.g. when adding an event to the thread, as will be done for each thread event received during a sync):

matrix-js-sdk/src/models/thread.ts

Lines 142 to 153 in dfb079a

	private async fetchRootEvent(): Promise<void> {
	this.rootEvent = this.room.findEventById(this.id);
	// If the rootEvent does not exist in the local stores, then fetch it from the server.
	try {
	const eventData = await this.client.fetchRoomEvent(this.roomId, this.id);
	const mapper = this.client.getEventMapper();
	this.rootEvent = mapper(eventData); // will merge with existing event object if such is known
	} catch (e) {
	logger.error("Failed to fetch thread root to construct thread with", e);
	}
	await this.processEvent(this.rootEvent);
	}

Note that this code both checks the cache for an event, then seems to indiscriminately fetch the root event from remote. According to the stack trace, this remote fetch is what leads to the redundant event subscriptions. (If I look at the network tab, I can see that there are many calls to /rooms/{roomId}/event/{eventId} for the same event, which I think is the thread root.)

I can't say for sure that this is the cause, but it at least seems like a bug in itself. If these are not related issues, I'd be happy to split this issue.

If this is a correct diagnostic, you should be able to reproduce the issue using this repo on an account that has a thread with at least 11 replies (11 = EventEmitter max listeners).

[davidisaaclee]

Here's a branch that I started that I think fixes the unnecessary fetch of the thread root: https://github.com/matrix-org/matrix-js-sdk/compare/develop...davidisaaclee:matrix-js-sdk:avoid-unnecessary-fetch-thread-root?expand=1

It's pretty tough to write a test around this code – I started a test a week ago and I forget the state of it. I can pick this back up later but thought I'd post what I have so far.

[davidisaaclee]

https://github.com/matrix-org/matrix-js-sdk/compare/develop...davidisaaclee:matrix-js-sdk:avoid-unnecessary-fetch-thread-root?expand=1 does not fix the MaxListenersExceededWarning:

• on initial sync, many messages will be added to a thread "at once"
• each of these will trigger a fetchRootEvent
• none of these will pass the this.rootEvent != null check, since they all start fetching at the same time
• they'll all start a request to fetch the root event

I'm not sure what the solution here is. Are there other similar situations in this library that already have solutions?

We could memoize the call to client.fetchRoomEvent, but this would be tricky, as I think fetchRoomEvent is not idempotent (e.g. server-side aggregated message edits).

It looks like there's a similar solution used for scrollbacks (actually, a couple of these stored promises in that file):

matrix-js-sdk/src/client.ts

Lines 5537 to 5540 in 9849818

	let info = this.ongoingScrollbacks[room.roomId] || {};
	if (info.promise) {
	return info.promise;
	} else if (info.errorTs) {

[davidisaaclee]

I tried reusing ongoing promises for thread.fetchRootEvent and thread.processRootEvent, which got rid of the unnecessary HTTP calls: davidisaaclee@891e2cb

But I still get MaxListenersExceededWarning. Looking at the new offending line of code, I see

matrix-js-sdk/src/models/event.ts

Lines 1617 to 1619 in 891e2cb

	if (thread) {
	this.reEmitter.reEmit(thread, [ThreadEvent.Update]);
	}

My reading of this code is that: for each event in a thread, that event re-emits ThreadEvent.Update events. The way this re-emit works for this concrete case is something like:

thread.addEventListener(ThreadEvent.Update, event => {
  this.reEmitter.emit(ThreadEvent.Update, event);
});

... which obviously leads to many event listeners (as many as there are processed events in the thread).

---

I don't get the design choice to re-emit ThreadEvent.Update on each event: any caller could just do someEvent.thread?.addEventListener(ThreadEvent.Update, ...). Would you consider a breaking change here to remove this re-emit?

[t3chguy]

Yes it'd be a breaking change but likely a welcome one

[davidisaaclee]

@t3chguy thank you – I've started the work here, which begins with reducing the unnecessary fetches of thread root events #3530

I plan on adding the code to remove the ThreadEvent.Update re-emit in the same PR unless you have objections; stepping away from computer for a few hours

[t3chguy]

@davidisaaclee no objections at this time

[davidisaaclee]

I'm giving up on the fixes in #3530 for now, since they are causing other issues: #3530 (comment) – the reported issues are still in the SDK, so I'd love if someone else wanted to try fixing.

[clokep]

Did #3541 fix this?

[t3chguy]

@clokep not fully, there's still redundant fetches happening where the data model is inadequate and asks the server for a re-up