Skip to content

Commit f408a51

Browse files
nreeseelasticmachinekibanamachine
authored
[embeddable] fix race condition in useBatchedPublishingSubjects (elastic#216399)
Closes elastic#214176 and elastic#214853 [upgrade from chrome 134 to 135 in functional test runner](elastic#213919) revealed a race condition in `useBatchedPublishingSubjects` where batched observables could emit new values before `useEffect` sets up the subscription. This PR resolves this issue by setting up subscription in useRef, which has no timing delays. In chrome 134, `useBatchedPublishingSubjects` `useEffect` gets called (setting up subscription) before lens embeddable emits any changes to batched observables. <img width="300" alt="chrome134" src="https://github.com/user-attachments/assets/b0356f74-e0c7-4d93-a23a-ace519194d5d" /> In chrome 135, `useBatchedPublishingSubjects` `useEffect` gets called after lens embeddable emits changes to batched observables. This causes the lens embeddable to not render since the `LensEmbeddableComponent` has a stale value for `expressionParams`. <img width="300" alt="chrome135" src="https://github.com/user-attachments/assets/320bfb7e-8b3f-4b48-a138-1c47c5ff9961" /> --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]>
1 parent bee34d5 commit f408a51

File tree

2 files changed

+52
-13
lines changed

2 files changed

+52
-13
lines changed

src/platform/packages/shared/presentation/presentation_publishing/publishing_subject/publishing_batcher.ts

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* License v3.0 only", or the "Server Side Public License, v 1".
88
*/
99

10-
import { useEffect, useRef, useState } from 'react';
10+
import { useEffect, useMemo, useRef, useState } from 'react';
1111
import { combineLatest, debounceTime, skip } from 'rxjs';
1212
import { AnyPublishingSubject, PublishingSubject, UnwrapPublishingSubjectTuple } from './types';
1313

@@ -120,22 +120,37 @@ export const useBatchedPublishingSubjects = <
120120

121121
/**
122122
* Subscribe to all subjects and update the latest values when any of them change.
123+
*
124+
* Can not set up subscription in useEffect.
125+
* useEffect introduces a race condition where subscriptions may emit after values are set with useState
126+
* but before subscription is setup in useEffect.
127+
*
128+
* Can not set up subscription in useRef.
129+
* useRef executes initialization logic every render.
123130
*/
124-
useEffect(() => {
125-
const subscription = combineLatest(subjects)
126-
.pipe(
127-
// When a new observer subscribes to a BehaviorSubject, it immediately receives the current value. Skip this emit.
128-
skip(1),
129-
debounceTime(0)
130-
)
131-
.subscribe((values) => {
132-
setLatestPublishedValues(values as UnwrapPublishingSubjectTuple<SubjectsType>);
133-
});
134-
return () => subscription.unsubscribe();
131+
const subscription = useMemo(
132+
() =>
133+
combineLatest(subjects)
134+
.pipe(
135+
// When a new observer subscribes to a BehaviorSubject, it immediately receives the current value. Skip this emit.
136+
skip(1),
137+
debounceTime(0)
138+
)
139+
.subscribe((values) => {
140+
setLatestPublishedValues(values as UnwrapPublishingSubjectTuple<SubjectsType>);
141+
}),
135142
// 'subjects' gets a new reference on each call because of spread
136143
// Use 'useBatchedOptionalPublishingSubjects' when 'subjects' are expected to change.
137144
// eslint-disable-next-line react-hooks/exhaustive-deps
138-
}, []);
145+
[]
146+
);
147+
148+
/**
149+
* Clean up subscription on unmount.
150+
*/
151+
useEffect(() => {
152+
return () => subscription.unsubscribe();
153+
}, [subscription]);
139154

140155
return latestPublishedValues;
141156
};

src/platform/packages/shared/presentation/presentation_publishing/publishing_subject/publishing_subject.test.tsx

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,30 @@ describe('publishing subject', () => {
118118
expect(renderCount).toBe(2);
119119
});
120120

121+
test('useBatchedPublishingSubjects should synchronously subscribe to observables to avoid race conditions', async () => {
122+
function Component() {
123+
const [value1] = useBatchedPublishingSubjects(subject1);
124+
125+
// synchronously emit new values for observables
126+
// this will cause test to fail if subscriptions are not setup synchronously
127+
incrementAll();
128+
129+
return (
130+
<>
131+
<span>{`value1: ${value1}`}</span>
132+
</>
133+
);
134+
}
135+
render(<Component />);
136+
await waitFor(() => {
137+
expect(
138+
// If there is a race condition, then 'value1: 0' will be rendered
139+
// because value1 will have the original value '0' instead of latest value
140+
screen.getByText('value1: 1')
141+
).toBeInTheDocument();
142+
});
143+
});
144+
121145
test('should batch state updates when using useBatchedOptionalPublishingSubjects', async () => {
122146
let renderCount = 0;
123147
function Component() {

0 commit comments

Comments
 (0)