Non-deterministic behavior / race condition in hooks

[ssyberg]

Link to reproduction

N/A

To Reproduce

This is a fairly complex set up and I haven't been able to reproduce it in a trivial example so I'm hoping something jumps out to whoever reviews this. I've encountered this bug a number of times over the last few months and because it's non-deterministic it's very hard to reliably reproduce.

Describe the Bug

I have two examples of this, one was where I setting a console.log within a beforeValidate hook would change the outcome of the validation (see this discord thread: https://discord.com/channels/967097582721572934/1091785894136139808/1091785894136139808).

More recently I've lost a few hours trying to debug this beforeChange hook and similarly hooks are firing and data is available non-deterministically:

name: 'path',
unique: true,
admin: {
    hidden: true,
},
hooks: {
    beforeChange: [
        ({ data }) => {
            if (data.slug && data.parent && data.parent.value) {
                return `${data.parent.value}/${data.slug}`;
            }
        },
    ],
},

Some important context:

• slug is being set in a beforeValidate hook
• parent is being set by a async method on defaultValue

This seems to reliably work in the admin UI, but never works on a local API create. When setting breakpoints in the above beforeChange hook I see data.slug is set correctly by it's beforeValidate hook, but data.parent is undefined.

FWIW I've tried different combos of beforeChange, beforeValidate, afterChange to address this and nothing seems to reliably solve it.

One important data point, adding a sleep completely solves this which makes me pretty confident it's a race condition:

hooks: {
    beforeChange: [
        async ({ data }) => {
            await new Promise((resolve) => setTimeout(resolve, 100));
        },
        ({ data }) => {
            if (data.slug && data.parent && data.parent.value) {
                return `${data.parent.value}/${data.slug}`;
            }
        },
    ],
},

Payload Version

1.8.3 (but have noticed this since 1.6.x)

[swenzel-arc]

If your field hooks depend on values of other fields that are themselves modified by their own field hooks, it might be wiser to merge all that logic into a collection level hook so you have full control of the order of execution and can edit all fields at the same place.

[ssyberg]

If your field hooks depend on values of other fields that are themselves modified by their own field hooks, it might be wiser to merge all that logic into a collection level hook so you have full control of the order of execution and can edit all fields at the same place.

That is a solution, though for my specific implementation this is occurring on an importable field group, what you're suggesting would mean importing the field group and setting up a collection hook on every collection that uses this which feels a bit brittle and not properly encapsulated.

That said I'd still expect inter-hook behavior to be deterministic, this still feels like a bug to me.

[jmikrut]

Let me chime in here quick with some additional info.

Field hooks are intentionally all run in parallel to one another for each step. So your parent field is calculating its defaultValue and your path field is attempting to read that value at the same time, in parallel (within the beforeChange lifecycle event / hook).

This is a performance optimization as well as a side-effect of the way that JavaScript works. The properties of field data are just keys of a plain old JavaScript object, which do not have intrinsic / reliable order. So even though we do map through the properties of your field data, the order is not predictable.

If you do need predictability in certain fields' calculations, then I would view the different hooks as the only reliable way to guarantee hook order. Or, as @swenzel-arc mentioned, just use a collection hook instead.

What would the alternative be here? I am certainly not opposed to building some type of "field hook ordering" system, but that would be more of a new feature and less of an issue with Payload as it stands now.

What do you think? I'll convert this to a discussion but if I have misunderstood something, and there is indeed a bug with Payload here, I'd be happy to re-open as an issue.

[ssyberg]

I think this still speaks to a odd divergence between admin and API behavior:

The beforeValidate hook runs BEFORE the defaultValue is calculated

For example in the dashboard the defaultValue of a select or radio will be preselected on the form on load and as such it's executed before the beforeValidate hook.

[ssyberg]

☝🏼 and I might add it's not obvious how to write hooks/defaults that play nicely in both scenarios.

[jmikrut]

For example in the dashboard the defaultValue of a select or radio will be preselected on the form on load and as such it's executed before the beforeValidate hook.

Well, remember that the beforeValidate hook doesn’t run at all in the browser though. So intrinsically the context is different.

In the admin UI, however, default values are indeed calculated before running validate functions - same as the server. So there is parity there.

and I might add it's not obvious how to write hooks/defaults that play nicely in both scenarios.

totally agreed with this and improving the docs for the Payload “lifecycle” is work that we are about to take on. You can expect more from us there shortly and that should help clarify a lot of this!

[jmikrut]

Oh also – we are about to release a feature that allows you to build “context“ for the hooks lifecycle, which will allow you to wait on certain actions to complete before running other hook actions. This might not be the most simple application of that new feature, and you might be better off trying to simplify rather than leverage the new hooks context here specifically, but thought I’d throw that out 👍

[ssyberg]

Cool thank you for all the responses! I am still curious what the downside is to running the defaultValue callback before any other hooks since that is still effectively what happens when it's "run in the browser"? From the servers perspective, I think it would make sense for the defaultValue to be treated as if it was coming from a PATCH/POST and that would address this behavior difference...