Await updateD1Data to avoid race conditions

[patdx]

I've been experimenting with a fork of this code, and noticed that sometimes my edits did not actually save in the production CF Workers environment. I could not reproduce it locally. It also did not seem to be cache related, even if I cleared the cache it still happened. I suspect it's related to this code:

sonicjs/src/cms/data/data.ts

Line 354 in 6426598

const result = updateD1Data(d1, data.table, data);

const result = updateD1Data(d1, data.table, data);

The function updateD1Data is async and returns a Promise, but the code seems to be written as if it is a synchronous function. Maybe if the HTTP request responds very quickly, the CF Workers runtime tries to suspend the worker isolate before the floating updateD1Data promise resolves, so the update does not actually get committed to the D1 Database.

Even if that is not actually the case, I think this is undefined behavior to have a floating promise to update the database. It may be a good idea to do either:

1. await the updateD1Data call in every instance that it is used, or
2. Call cloudflare's context.waitUntil eg: context.waitUntil(updateD1Data(d1, data.table, data))

I did strategy 1 in my fork and it seemed to resolve the issue. But it's hard to say 100% as it only happened in CF Workers, not locally.

[lane711]

Hi Patrick, thank you for the great research here. #1 should work. We're doing an await on the insert so adding it on the update was probably just something that was missed. I'd be happy to approve a PR if you'd like to submit, else I can make that change. Again, thanks for bringing this to my attention

…

On Thu, Dec 14, 2023 at 2:34 AM Patrick Miller ***@***.***> wrote: I've been experimenting with a fork of this code, and noticed that sometimes my edits did not actually save in the production environment. I could not reproduce it locally. It also did not seem to be cache related, even if I cleared the cache it still happened. I suspect it's related to this code: https://github.com/lane711/sonicjs/blob/642659893008bc627cd2fdd30e827f50f953a95f/src/cms/data/data.ts#L354 const result = updateD1Data(d1, data.table, data); The function updateD1Data is async and returns a Promise, but the code seems to be written as if it is a synchronous function. Maybe if the HTTP request responds very quickly, the CF Workers runtime tries to suspend the worker isolate before the floating updateD1Data promise resolves, so the update does not actually get committed to the D1 Database. Even if that is not actually the case, I think this is undefined behavior to have a floating promise to update the database. It may be a good idea to do either: 1. await the updateD1Data call in every instance that it is used, or 2. Call cloudflare's context.waitUntil <https://developers.cloudflare.com/workers/runtime-apis/handlers/fetch/#contextwaituntil> eg: context.waitUntil(updateD1Data(d1, data.table, data)) I did strategy 1 in my fork and it seemed to resolve the issue. — Reply to this email directly, view it on GitHub <#217>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBGCC56VEXXKQCYCM2AQ3LYJLI3HAVCNFSM6AAAAABAUTPVFWVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA2DCNBRGIZTQMI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[patdx]

Hi @lane711 sorry for the long delay. Between end of year holidays and other responsibilities I didn't have time to look at this for a while. Finally I found some time to catch up on my GitHub backlog this week. I submitted a PR for the issue above. It's a very simple PR. Let me know if you need anything else and I'll try to adjust it. If you would like to resolve the issue faster please feel free to make any necessary changes yourself, I won't mind. Thanks! #227