8000 Fix _processing flag in BaseClient. (#2983) · szechyjs/sentry-javascript@7fc797b · GitHub
[go: up one dir, main page]
Skip to content

Commit 7fc797b

Browse files
marshall-leekamilogorek
marshall-lee
and
kamilogorek
authored
Fix _processing flag in BaseClient. (getsentry#2983)
Client processes events concurrently so it should be a counter, not a boolean. Co-authored-by: Kamil Ogórek <kamil.ogorek@gmail.com>
1 parent 18bce3c commit 7fc797b

File tree

2 files changed

+99
-43
lines changed

2 files changed

+99
-43
lines changed

‎packages/core/src/baseclient.ts

Lines changed: 68 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
7575
/** Array of used integrations. */
7676
protected _integrations: IntegrationIndex = {};
7777

78-
/** Is the client still processing a call? */
79-
protected _processing: boolean = false;
78+
/** Number of call being processed */
79+
protected _processing: number = 0;
8080

8181
/**
8282
* Initializes this client instance.
@@ -99,14 +99,15 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
9999
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
100100
public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined {
101101
let eventId: string | undefined = hint && hint.event_id;
102-
this._processing = true;
103102

104-
// eslint-disable-next-line @typescript-eslint/no-floating-promises
105-
this._getBackend()
106-
.eventFromException(exception, hint)
107-
.then(event => {
108-
eventId = this.captureEvent(event, hint, scope);
109-
});
103+
this._process(
104+
this._getBackend()
105+
.eventFromException(exception, hint)
106+
.then(event => this._captureEvent(event, hint, scope))
107+
.then(result => {
108+
eventId = result;
109+
}),
110+
);
110111

111112
return eventId;
112113
}
@@ -116,16 +117,18 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
116117
*/
117118
public captureMessage(message: string, level?: Severity, hint?: EventHint, scope?: Scope): string | undefined {
118119
let eventId: string | undefined = hint && hint.event_id;
119-
this._processing = true;
120120

121121
const promisedEvent = isPrimitive(message)
122122
? this._getBackend().eventFromMessage(`${message}`, level, hint)
123123
: this._getBackend().eventFromException(message, hint);
124124

125-
// eslint-disable-next-line @typescript-eslint/no-floating-promises
126-
promisedEvent.then(event => {
127-
eventId = this.captureEvent(event, hint, scope);
128-
});
125+
this._process(
126+
promisedEvent
127+
.then(event => this._captureEvent(event, hint, scope))
128+
.then(result => {
129+
eventId = result;
130+
}),
131+
);
129132

130133
return eventId;
131134
}
@@ -135,17 +138,12 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
135138
*/
136139
public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined {
137140
let eventId: string | undefined = hint && hint.event_id;
138-
this._processing = true;
139141

140-
this._processEvent(event, hint, scope)
141-
.then(finalEvent => {
142-
eventId = finalEvent.event_id;
143-
this._processing = false;
144-
})
145-
.then(null, reason => {
146-
logger.error(reason);
147-
this._processing = false;
148-
});
142+
this._process(
143+
this._captureEvent(event, hint, scope).then(result => {
144+
eventId = result;
145+
}),
146+
);
149147

150148
return eventId;
151149
}
@@ -179,12 +177,11 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
179177
* @inheritDoc
180178
*/
181179
public flush(timeout?: number): PromiseLike<boolean> {
182-
return this._isClientProcessing(timeout).then(status => {
183-
clearInterval(status.interval);
180+
return this._isClientProcessing(timeout).then(ready => {
184181
return this._getBackend()
185182
.getTransport()
186183
.close(timeout)
187-
.then(transportFlushed => status.ready && transportFlushed);
184+
.then(transportFlushed => ready && transportFlushed);
188185
});
189186
}
190187

@@ -263,30 +260,23 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
263260
}
264261

265262
/** Waits for the client to be done with processing. */
266-
protected _isClientProcessing(timeout?: number): PromiseLike<{ ready: boolean; interval: number }> {
267-
return new SyncPromise<{ ready: boolean; interval: number }>(resolve => {
263+
protected _isClientProcessing(timeout?: number): PromiseLike<boolean> {
264+
return new SyncPromise(resolve => {
268265
let ticked: number = 0;
269266
const tick: number = 1;
270267

271-
let interval = 0;
272-
clearInterval(interval);
273-
274-
interval = (setInterval(() => {
275-
if (!this._processing) {
276-
resolve({
277-
interval,
278-
ready: true,
279-
});
268+
const interval = setInterval(() => {
269+
if (this._processing == 0) {
270+
clearInterval(interval);
271+
resolve(true);
280272
} else {
281273
ticked += tick;
282274
if (timeout && ticked >= timeout) {
283-
resolve({
284-
interval,
285-
ready: false,
286-
});
275+
clearInterval(interval);
276+
resolve(false);
287277
}
288278
}
289-
}, tick) as unknown) as number;
279+
}, tick);
290280
});
291281
}
292282

@@ -455,6 +445,24 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
455445
this._getBackend().sendEvent(event);
456446
}
457447

448+
/**
449+
* Processes the event and logs an error in case of rejection
450+
* @param event
451+
* @param hint
452+
* @param scope
453+
*/
454+
protected _captureEvent(event: Event, hint?: EventHint, scope?: Scope): PromiseLike<string | undefined> {
455+
return this._processEvent(event, hint, scope).then(
456+
finalEvent => {
457+
return finalEvent.event_id;
458+
},
459+
reason => {
460+
logger.error(reason);
461+
return undefined;
462+
},
463+
);
464+
}
465+
458466
/**
459467
* Processes an event (either error or message) and sends it to Sentry.
460468
*
@@ -537,4 +545,21 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
537545
);
538546
});
539547
}
548+
549+
/**
550+
* Occupies the client with processing and event
551+
*/
552+
protected _process<T>(promise: PromiseLike<T>): void {
553+
this._processing += 1;
554+
promise.then(
555+
value => {
556+
this._processing -= 1;
557+
return value;
558+
},
559+
reason => {
560+
this._processing -= 1;
561+
return reason;
562+
},
563+
);
564+
}
540565
}

‎packages/core/test/lib/base.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -843,6 +843,37 @@ describe('BaseClient', () => {
843843
expect(transportInstance.sendCalled).toEqual(1);
844844
});
845845

846+
test('flush with some events being processed async', async () => {
847+
jest.useRealTimers();
848+
expect.assertions(5);
849+
const client = new TestClient({
850+
dsn: PUBLIC_DSN,
851+
enableSend: true,
852+
transport: FakeTransport,
853+
});
854+
855+
const delay = 300;
856+
const spy = jest.spyOn(TestBackend.instance!, 'eventFromMessage');
857+
spy.mockImplementationOnce(
858+
(message, level) =>
859+
new SyncPromise((resolve, _reject) => {
860+
setTimeout(() => resolve({ message, level }), 150);
861+
}),
862+
);
863+
const transportInstance = (client as any)._getBackend().getTransport() as FakeTransport;
864+
transportInstance.delay = delay;
865+
866+
client.captureMessage('test async');
867+
client.captureMessage('test non-async');
868+
expect(transportInstance).toBeInstanceOf(FakeTransport);
869+
expect(transportInstance.sendCalled).toEqual(1);
870+
expect(transportInstance.sentCount).toEqual(0);
871+
await client.flush(delay);
872+
expect(transportInstance.sentCount).toEqual(2);
873+
expect(transportInstance.sendCalled).toEqual(2);
874+
spy.mockRestore();
875+
});
876+
846877
test('close', async () => {
847878
jest.useRealTimers();
848879
expect.assertions(2);

0 commit comments

Comments
 (0)
0