Skip to content

Commit 946b844

Browse files
arturovtmmalerba
authored andcommitted
fix(core): async EventEmitter error should not prevent stability (#61028)
This commit wraps the `fn` invocation with `try-finally`, ensuring that the pending task (added in [this commit](d5c6ee4)) is always removed. Prior to this commit, if a subscriber threw an error, it would prevent the application from becoming stable — though this shouldn't happen under normal scenarios because the error should be handled by the RxJS error handler or Angular's error handler. Errors should not silently prevent the application from being rendered on the server. PR Close #61028
1 parent 222ae18 commit 946b844

File tree

2 files changed

+23
-5
lines changed

2 files changed

+23
-5
lines changed

packages/core/src/event_emitter.ts

+6-3
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,12 @@ class EventEmitter_ extends Subject<any> implements OutputRef<any> {
176176
return (value: unknown) => {
177177
const taskId = this.pendingTasks?.add();
178178
setTimeout(() => {
179-
fn(value);
180-
if (taskId !== undefined) {
181-
this.pendingTasks?.remove(taskId);
179+
try {
180+
fn(value);
181+
} finally {
182+
if (taskId !== undefined) {
183+
this.pendingTasks?.remove(taskId);
184+
}
182185
}
183186
});
184187
};

packages/core/test/event_emitter_spec.ts

+17-2
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@
77
*/
88

99
import {TestBed} from '../testing';
10-
import {filter, tap} from 'rxjs/operators';
10+
import {filter} from 'rxjs/operators';
1111

1212
import {EventEmitter} from '../src/event_emitter';
13-
import {ApplicationRef} from '../public_api';
13+
import {ApplicationRef, NgZone} from '../public_api';
1414

1515
describe('EventEmitter', () => {
1616
let emitter: EventEmitter<number>;
@@ -206,6 +206,21 @@ describe('EventEmitter', () => {
206206
expect(emitValue!).toEqual(1);
207207
});
208208

209+
it('should not prevent app from becoming stable if subscriber throws an error', async () => {
210+
const logs: string[] = [];
211+
const ngZone = TestBed.inject(NgZone);
212+
const appRef = TestBed.inject(ApplicationRef);
213+
appRef.isStable.subscribe((isStable) => logs.push(`isStable=${isStable}`));
214+
const emitter = TestBed.runInInjectionContext(() => new EventEmitter<number>(true));
215+
emitter.subscribe(() => {
216+
throw new Error('Given this is some TypeError...');
217+
});
218+
// Emit inside the Angular zone so that the error is not captured by Jasmine in `afterAll`.
219+
ngZone.run(() => emitter.emit(1));
220+
await appRef.whenStable();
221+
expect(logs).toEqual(['isStable=true', 'isStable=false', 'isStable=true']);
222+
});
223+
209224
// TODO: vsavkin: add tests cases
210225
// should call dispose on the subscription if generator returns {done:true}
211226
// should call dispose on the subscription on throw

0 commit comments

Comments
 (0)