fix: race condition when using state changing events

[oscb]

• fix: upgrade sovran to v0.3.0, add concurrency safe getters
• fix: support async processing in timeline
• fix: make all client methods awaitable, fix userInfo race condition
• chore: cleanup

[alanjcharles]

lgtm!

[509dave16]

@oscb Can you do a release for these changes today or tomorrow? The userInfo race condition will break our analytics models and other downstream destinations.

[oscb]

🎉 This PR is included in version @segment/analytics-react-native-v2.5.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[oscb]

🎉 This PR is included in version @segment/analytics-react-native-plugin-amplitude-session-v0.3.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[buuuudzik]

Adding async without await has a limited sense. It only moves the execution of client.screen() after the end of the current synchronous code but it doesn't allow to fully control the order of multiple concurrent event calls.

[oscb]

These are the public exposed methods of the client. They are now marked as async exactly for that reason. Their execution has always been asynchronous but wasn't previously awaitable as it didn't return the promise. Now it does. We don't await for this because we are not the ones calling them. But you can if you need to ensure a call completes before continuing execution.

…

On Aug 11, 2022, 22:06 +0100, slawomir.budzynski ***@***.***>, wrote: @buuuudzik commented on this pull request. In packages/core/src/client.tsx: > @@ -61,12 +61,12 @@ export const useAnalytics = (): ClientMethods => { return {}; } return { - screen: (...args) => client.screen(...args), - track: (...args) => client.track(...args), - identify: (...args) => client.identify(...args), - flush: () => client.flush(), - group: (...args) => client.group(...args), - alias: (...args) => client.alias(...args), - reset: (...args) => client.reset(...args), + screen: async (...args) => client.screen(...args), Adding async without await has a limited sense. It only moves the execution of client.screen() after the end of the current synchronous code but it doesn't allow to fully control the order of multiple concurrent event calls. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[buuuudzik]

In JS async isn't only a mark, it has a significant influence on the order of a code execution. And returning a promise doesn't mean that the function is asynchronous.

In analytics.js all looks ok and async+await has a lot of sense and allows to precisely wait until specific async function will end.

async identify(userId, userTraits) {
    const userData = await this.store.userInfo.set(state => ({ ...state,
      userId: userId ?? state.userId,
      traits: { ...state.traits,
        ...userTraits
      }
    }));
    const event = createIdentifyEvent({
      userId: userData.userId,
      userTraits: userData.traits
    });
    await this.process(event);
    this.logger.info('IDENTIFY event saved', event);
}

But in client.js and the object returned by useAnalytics hook we don't need to wait for returning promise. And the result of await will be still another promise which needs to be handled in such a way.

const { screen } = useAnalytics();

const onClick = async () => {
  const promise = await screen();
  await promise;
  console.log("100% after screen")
}

It will have a sense only with such a code:

screen: async (...args) => await client.screen(...args),

Another thing is that is this really necessary to transforming the whole API to async? It looks that only the storage persistence and sending events over http are async. But adding events to the queue and storing userData in RAM for consecutive calls could be fully sync.

[buuuudzik]

Sorry, I've checked and I see that it works in a current Chrome also with only returning a promise. But still I don't know why async API is necessary for handling events locally.

[oscb]

It's really async, it's not just returning a promise. The execution is async all the way down. If you await it you are actually waiting for the event to go through the whole timeline processing where it executes all the plugins. It has always been this way however the original design just fired the processing without letting the caller await for it. That said I get the issue, it looks like we are double wrapping the promise which is incorrect. We need to fix that. About why it required making everything async: I think it is about making it clearer to the caller that triggering an event can have asynchronous effects. It's up to the caller to decide if the order of execution is important or not, instead of us deciding that it is not (which was the original problem). Not everything can be sync because events are not just queued, they get through a timeline of plugins which can in turn have their own side effects as well as do any operation (native calls for instance) that we have to account for. It makes things more complex for us but it is all in favor of keeping things flexible.

…

On Aug 12, 2022, 09:56 +0100, slawomir.budzynski ***@***.***>, wrote: In JS async isn't only a mark, it has significant influence on the order of a code execution. And returning a promise doesn't mean that the function is asynchronous. In analytics.js all looks ok and async+await has a lot of sense and allows to precisely wait until specific async function will end. async identify(userId, userTraits) { const userData = await this.store.userInfo.set(state => ({ ...state, userId: userId ?? state.userId, traits: { ...state.traits, ...userTraits } })); const event = createIdentifyEvent({ userId: userData.userId, userTraits: userData.traits }); await this.process(event); this.logger.info('IDENTIFY event saved', event); } But in client.js and the object returned by useAnalytics hook we don't need to wait for returning promise. And the result of await will be still another promise. ` const { screen } = useAnalytics(); const onClick = async () => { const promise = await screen(); await promise; console.log("100% after screen") } ` It will have a sense only with such a code: screen: async (...args) => await client.screen(...args), Another thing is that is this really necessary to transforming whole API to async? It looks that only the storage persistence and sending events over http is async. But adding events to the queue and storing userData in RAM for consecutive calls could be fully sync. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[buuuudzik]

Thanks @oscb for this exhaustive explanation and maybe the plugins part could be unpredictable.

But really in my opinion segment library should have an option to handle synchronously event queue because using async/await equals that events will receive later timestamps and it works but only in the same async function, it's much harder to sync events executed from totally different app places.

For me at least .identify() call should be synchronous or asynchronous but the userId passed by this event should be immediately used in further events.

You wrote:
'It's up to the caller to decide if the order of execution is important or not, instead of us deciding that it is not (which was the original problem).'

Probably what is the most important for the users is:

• createdAt timestamp,
• immediate usage of the userData passed by .identify().

If Segment would not handle userData by .identify() call and all events payload would be prepared by the user then .track() calls order would be not important. This is the case.