basarat / basarat/typescript-book

Bug in TypedEvent causes listeners to not fire in specific circumstances

Open
#405 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
21.6k
Forks
2.6k
PR merge metrics
No merged PRs in 30d

Description

Hello -
Thank to everyone for all your effort in putting this together. It has been incredibly useful in spinning up and learning Typescript for me recently.

I wanted to bring your attention to a bug I discovered in the TypedEvent tip.

If one of the listeners calls `off` on a listener that has already been run, you will (quite unexpectedly) skip event listeners. This is due to the fact that the listeners array is not being cloned before firing.

Additionally, if a listener (or a once) adds a 'once' handler during execution, it will be executed immediately and the total list will be truncated.

Both of these were _very_ difficult issues to track down, so I was hoping to prevent anyone else the trouble.

Below is a snippet demonstrating the problem ([running example](https://www.typescriptlang.org/play/#src=type%20IListener%3CT%3E%20%3D%20(event%3A%20T)%20%3D%3E%20any%3B%0D%0A%0D%0Ainterface%20IDisposable%20%7B%0D%0A%20%20%20%20dispose()%20%3A%20void%3B%0D%0A%7D%0D%0A%0D%0A%2F**%20passes%20through%20events%20as%20they%20happen.%20You%20will%20not%20get%20events%20from%20before%20you%20start%20listening%20*%2F%0D%0Aclass%20TypedEvent%3CT%20%3D%7B%7D%3E%20%7B%0D%0A%20%20%20%20private%20listeners%3A%20Array%3CIListener%3CT%3E%3E%20%3D%20%5B%5D%3B%0D%0A%20%20%20%20private%20listenersOncer%3A%20Array%3CIListener%3CT%3E%3E%20%3D%20%5B%5D%3B%0D%0A%20%20%20%20%0D%0A%20%20%20%20public%20on%20%3D%20(listener%3A%20IListener%3CT%3E)%3A%20IDisposable%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20this.listeners.push(listener)%3B%0D%0A%20%20%20%20%20%20%20%20return%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20dispose%3A%20()%20%3D%3E%20this.off(listener)%0D%0A%20%20%20%20%20%20%20%20%7D%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20public%20once%20%3D%20(listener%3A%20IListener%3CT%3E)%3A%20void%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20this.listenersOncer.push(listener)%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20public%20off%20%3D%20(listener%3A%20IListener%3CT%3E)%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20const%20callbackIndex%20%3D%20this.listeners.indexOf(listener)%3B%0D%0A%20%20%20%20%20%20%20%20if%20(callbackIndex%20%3E%20-1)%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20this.listeners.splice(callbackIndex%2C%201)%3B%0D%0A%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20public%20emit%20%3D%20(event%3A%20T)%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20%2F**%20Update%20any%20general%20listeners%20*%2F%0D%0A%20%20%20%20%20%20%20%20this.listeners.forEach((listener)%20%3D%3E%20listener(event))%3B%0D%0A%0D%0A%20%20%20%20%20%20%20%20%2F**%20Clear%20the%20%60once%60%20queue%20*%2F%0D%0A%20%20%20%20%20%20%20%20this.listenersOncer.forEach((listener)%20%3D%3E%20listener(event))%3B%0D%0A%20%20%20%20%20%20%20%20this.listenersOncer%20%3D%20%5B%5D%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20public%20pipe%20%3D%20(te%3A%20TypedEvent%3CT%3E)%3A%20IDisposable%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20return%20this.on((e)%20%3D%3E%20te.emit(e))%3B%0D%0A%20%20%20%20%7D%0D%0A%7D%0D%0A%0D%0Aclass%20TypedEventWithFix%3CT%20%3D%7B%7D%3E%20%7B%0D%0A%20%20%20%20private%20listeners%3A%20Array%3CIListener%3CT%3E%3E%20%3D%20%5B%5D%3B%0D%0A%20%20%20%20private%20listenersOncer%3A%20Array%3CIListener%3CT%3E%3E%20%3D%20%5B%5D%3B%0D%0A%20%20%20%20%0D%0A%20%20%20%20public%20on%20%3D%20(listener%3A%20IListener%3CT%3E)%3A%20IDisposable%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20this.listeners.push(listener)%3B%0D%0A%20%20%20%20%20%20%20%20return%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20dispose%3A%20()%20%3D%3E%20this.off(listener)%0D%0A%20%20%20%20%20%20%20%20%7D%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20public%20once%20%3D%20(listener%3A%20IListener%3CT%3E)%3A%20void%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20this.listenersOncer.push(listener)%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20public%20off%20%3D%20(listener%3A%20IListener%3CT%3E)%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20const%20callbackIndex%20%3D%20this.listeners.indexOf(listener)%3B%0D%0A%20%20%20%20%20%20%20%20if%20(callbackIndex%20%3E%20-1)%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20this.listeners.splice(callbackIndex%2C%201)%3B%0D%0A%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20public%20emit%20%3D%20(event%3A%20T)%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20const%20listeners%20%3A%20Array%3C(event%20%3A%20T)%20%3D%3E%20void%3E%20%20%3D%20Object.assign(%5B%5D%2C%20this.listeners)%3B%0D%0A%0D%0A%20%20%20%20%20%20%20%20%2F**%20Update%20any%20general%20listeners%20*%2F%0D%0A%20%20%20%20%20%20%20%20listeners.forEach((listener)%20%3D%3E%20listener(event))%3B%0D%0A%0D%0A%20%20%20%20%20%20%20%20const%20oncers%20%3A%20%20Array%3C(event%20%3A%20T)%20%3D%3E%20void%3E%20%20%3D%20Object.assign(%5B%5D%2C%20this.listenersOncer)%3B%0D%0A%20%20%20%20%20%20%20%20this.listenersOncer%20%3D%20%5B%5D%3B%0D%0A%0D%0A%20%20%20%20%20%20%20%20%2F**%20Clear%20the%20%60once%60%20queue%20*%2F%0D%0A%20%20%20%20%20%20%20%20oncers.forEach((listener)%20%3D%3E%20listener(event))%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20public%20pipe%20%3D%20(te%3A%20TypedEvent%3CT%3E)%3A%20IDisposable%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20return%20this.on((e)%20%3D%3E%20te.emit(e))%3B%0D%0A%20%20%20%20%7D%0D%0A%7D%0D%0A%0D%0A%0D%0Aconst%20otherListener%20%3D%20()%20%3D%3E%20console.log(%22Hello%20World%22)%3B%0D%0Aconst%20listeners%20%3D%20%5B%0D%0A%20%20%20%20()%20%3D%3E%20%7B%20console.log('1')%3B%20typedEvent.off(otherListener)%20%7D%2C%0D%0A%20%20%20%20()%20%3D%3E%20console.log('2')%2C%0D%0A%20%20%20%20()%20%3D%3E%20console.log('3')%2C%0D%0A%20%20%20%20()%20%3D%3E%20console.log('4')%2C%0D%0A%20%20%20%20()%20%3D%3E%20console.log('5')%0D%0A%5D%3B%0D%0A%0D%0A%0D%0Aconst%20typedEvent%20%3D%20new%20TypedEvent()%3B%0D%0AtypedEvent.on(otherListener)%3B%0D%0Alisteners.forEach(l%20%3D%3E%20typedEvent.on(l))%3B%0D%0A%0D%0AtypedEvent.emit(%7B%7D)%3B%0D%0A%0D%0A%0D%0A%2F*%20Emits%20to%20the%20console%3A%20%0D%0AHello%20World%0D%0A1%0D%0A3%0D%0A4%0D%0A5%0D%0A*%2F%0D%0A%0D%0A%0D%0Aconst%20typedEventWithFix%20%3D%20new%20TypedEventWithFix()%3B%0D%0A%0D%0AtypedEventWithFix.on(otherListener)%3B%0D%0Alisteners.forEach(l%20%3D%3E%20typedEventWithFix.on(l))%3B%0D%0A%0D%0AtypedEventWithFix.emit(%7B%7D)%3B%0D%0A%0D%0A%2F*%20Emits%20to%20the%20console%3A%0D%0AHello%20World%0D%0A1%0D%0A2%0D%0A3%0D%0A4%0D%0A5%0D%0A*%2F) that prints to console).

here is the original emit method from your tip.
```
public emit = (event: T) => {
/** Update any general listeners */
this.listeners.forEach((listener) => listener(event));

/** Clear the once queue */
this.listenersOncer.forEach((listener) => listener(event));
this.listenersOncer = [];
}
```

By configuring the listeners as follows
```
const otherListener = () => console.log("Hello World");
const listeners = [
() => { console.log('1'); typedEvent.off(otherListener) },
() => console.log('2'),
() => console.log('3'),
() => console.log('4'),
() => console.log('5')
];

const typedEvent = new TypedEvent();
typedEvent.on(otherListener);
listeners.forEach(l => typedEvent.on(l));
```
and then emitting the event
```
typedEvent.emit({});
```

We see the following console output:
Hello World
1
3
4
5

Notice the missing `2`

By changing the emit in the following way
```
public emit = (event: T) => {
// grab a copy of the listeners and once arrays so that event handlers that
// interact with this don't interfere with the current execution enumeration
const listeners : Array<(event : T) => void> = Object.assign([], this.listeners);
const oncers : Array<(event : T) => void> = Object.assign([], this.listenersOncer);
this.listenersOncer = [];

/** Update any general listeners */
listeners.forEach((listener) => listener(event));

/** Clear the once queue */
oncers.forEach((listener) => listener(event));
}
```

and configuring the listeners exactly the same
```
const typedEventWithFix = new TypedEventWithFix();

typedEventWithFix.on(otherListener);
listeners.forEach(l => typedEventWithFix.on(l));
```

emitting the event (`typedEventWithFix.emit({});`) now correctly fires all the listeners:

Hello World
1
2
3
4
5

Thanks!

Contributor guide

Open the contributing guide

Research direction

Locate the TypedEvent tip and its emit method, then review the listener and once-handler behavior described in the issue. Reproduce the missing-listener case and the handler-added-during-execution case from the supplied snippets. Done means the tip's example behaves like the TypedEventWithFix output and all listeners fire as expected.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
documentation
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.