Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useObject inside FlatList throws Exception in HostFunction: Cannot create asynchronous query while in a write transaction #4375

Closed
mfbx9da4 opened this issue Feb 25, 2022 · 24 comments
Assignees

Comments

@mfbx9da4
Copy link

mfbx9da4 commented Feb 25, 2022

How frequently does the bug occur?

All the time

Description

Calling useObject inside a flat list item component can throw the Exception "Cannot create asynchronous query while in a write transaction" if a realm.write() occurs around the same time.

My guess cause is that the FlatList is doing some multithreading work to do the layouting of the item. As you will see in my example, the offending writes do not call addListener inside the transaction.

Stacktrace & log output

Error: Exception in HostFunction: Cannot create asynchronous query while in a write transaction

This error is located at:
    in Message (at App.tsx:121)
    in RCTView (at View.js:32)
    in View (at VirtualizedList.js:2073)
    in VirtualizedListCellContextProvider (at VirtualizedList.js:2088)
    in CellRenderer (at VirtualizedList.js:814)
    in RCTScrollContentView (at ScrollView.js:1674)
    in RCTScrollView (at ScrollView.js:1792)
    in ScrollView (at ScrollView.js:1818)
    in ScrollView (at VirtualizedList.js:1268)
    in VirtualizedListContextProvider (at VirtualizedList.js:1100)
    in VirtualizedList (at FlatList.js:645)
    in FlatList (at App.tsx:118)
    in RCTSafeAreaView (at SafeAreaView.js:51)
    in SafeAreaView (at App.tsx:116)
    in FooList (at App.tsx:155)
    in App (at renderApplication.js:50)
    in RCTView (at View.js:32)
    in View (at AppContainer.js:92)
    in RCTView (at View.js:32)
    in View (at AppContainer.js:119)
    in AppContainer (at renderApplication.js:43)
    in Onin(RootComponent) (at renderApplication.js:60)

Can you reproduce the bug?

Yes, always

Reproduction Steps

import React, { useEffect, useState } from 'react'
import { FlatList, SafeAreaView, Text } from 'react-native'
import RNFS from 'react-native-fs'
import Realm from 'realm'

export function App() {
  const r = useWaitForRealm()
  const [initialized, setInitialized] = useState(false)

  useEffect(() => {
    if (!r) return
    const asyncEffect = () => {
      // cleanup the db
      const fooResults = realm.objects<Foo>('foo')
      realm.write(() => {
        for (const x of fooResults) {
          realm.delete(x)
        }
      })
      setInitialized(true)
    }
    void asyncEffect()
  }, [r])

  if (!initialized) return null

  return <FooList />
}

let i = 0
const sleep = (milliseconds: number) => new Promise(r => setTimeout(r, milliseconds))
function FooList() {
  const fooResults = useQuery<Foo>(() => realm.objects<Foo>('foo'))

  useEffect(() => {
    const asyncEffect = async () => {
      while (i < 30) {
        console.log('i', i)
        await sleep(10)
        const id = String(i++)
        realm.write(() => {
          realm.create<Foo>('foo', { id }, Realm.UpdateMode.Modified)
        })
        await sleep(0)
        realm.write(() => {
          realm.create<Foo>('foo', { id }, Realm.UpdateMode.Modified)
        })
      }
    }
    asyncEffect().catch(console.error)
  }, [])

  return (
    <SafeAreaView style={{ margin: 20 }}>
      <Text>{fooResults?.length}</Text>
      <FlatList
        inverted
        data={fooResults}
        renderItem={() => <Message />}
        keyExtractor={item => item.id}
        maintainVisibleContentPosition={{ minIndexForVisible: 0, autoscrollToTopThreshold: 500 }}
      />
    </SafeAreaView>
  )
}

function Message() {
  const x = useObject<Foo>('foo', '0')
  return <Text>{x?.id}</Text>
}

// #region === Setup the Realm instance (start) ===
// You can skip reading this bit, I've left it here so it can be easily reproduced.
const FooSchema: Realm.ObjectSchema = {
  name: 'foo',
  primaryKey: 'id',
  properties: {
    id: 'string',
  },
}

export let realm: Realm
let realmInitializingPromise: Promise<Realm> | undefined
export function waitForRealm() {
  if (realm) return Promise.resolve(realm)
  if (!realmInitializingPromise) realmInitializingPromise = initRealm()
  return realmInitializingPromise
}

async function initRealm() {
  const path = `${RNFS.CachesDirectoryPath}/example.realm`
  realm = await Realm.open({
    path,
    schema: [FooSchema],
    schemaVersion: 0,
  })
  return realm
}

export function useWaitForRealm() {
  const [optionalRealm, setRealm] = useState<Realm | undefined>(realm)
  useEffect(() => {
    waitForRealm()
      .then(x => setRealm(x))
      .catch(console.error)
  }, [])
  return optionalRealm
}

type Foo = { id: string }

export function useObject<T>(type: string, primaryKey: string): (T & Realm.Object) | undefined {
  const [object, setObject] = useState<(T & Realm.Object) | undefined>(
    realm.objectForPrimaryKey(type, primaryKey)
  )

  useEffect(() => {
    const listenerCallback: Realm.ObjectChangeCallback = (_, changes) => {
      if (changes.changedProperties.length > 0) {
        setObject(realm.objectForPrimaryKey(type, primaryKey))
      } else if (changes.deleted) {
        setObject(undefined)
      }
    }
    if (object !== undefined) {
      object.addListener(listenerCallback)
    }
    return () => {
      object?.removeListener(listenerCallback)
    }
  }, [object, type, primaryKey])

  return object
}

function useQuery<T>(query: () => Realm.Results<any>) {
  const [collection, setCollection] = useState<Realm.Results<T>>(query())

  useEffect(() => {
    const listenerCallback: Realm.CollectionChangeCallback<T> = (_, changes) => {
      const { deletions, insertions, newModifications } = changes
      if (deletions.length > 0 || insertions.length > 0 || newModifications.length > 0) {
        setCollection(query())
      }
    }

    if (collection && collection.isValid() && !realm.isClosed)
      collection.addListener(listenerCallback)

    return () => {
      collection?.removeListener(listenerCallback)
    }
  }, [collection])

  return collection
}
// #endregion === Setup the Realm instance (end) ===

Version

"realm": "^10.20.0-beta.1",

What SDK flavour are you using?

Local Database only

Are you using encryption?

No, not using encryption

Platform OS and version(s)

ios

Build environment

No response

Cocoapods version

  • RealmJS (10.20.0-beta.1):
@takameyer
Copy link
Contributor

@mfbx9da4 Thanks for reporting. We will try to reproduce this and get back to you soon.

@takameyer
Copy link
Contributor

@mfbx9da4 Just wanted to update, it seems the async nature of how the writes are happening are causing a bit of a race condition where the write transaction is started in the event loop, then the main thread renders useObject which sets an event listener and causes the error. It's a bit tricky to fix, but we are looking into it.

@takameyer
Copy link
Contributor

@mfbx9da4 Been talking to other members of the team about this. We currently have an open feature request for async transactions in realm (see #1099). Currently, we cannot guarantee that async () => realm.write(() => …) will work as expected. We will be looking into adding support for this in the near future.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 2, 2022

the write transaction is started in the event loop, then the main thread renders useObject

Do you mean the main UI thread of the native platform or main JS thread? Assuming main JS thread, I still don't see how the write transaction isn't closed before anything else on the main JS thread is executed since JS is single threaded and AFIK, a write transaction callback is synchronous.

Do you mean, under the hood it goes something like this

realm.write = (callback) => {
  beginTransaction()
  setTimeout(() => {
    callback()
    endTransaction()
  })
}

In which case why is the callback enqueued onto the event loop? Why not do it synchronously?

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 2, 2022

We currently have an open feature request for async transactions in realm (see #1099). Currently, we cannot guarantee that async () => realm.write(() => …) will work as expected.

Having read #1099, I can see realm.write(async () => ... is not supported but I don't see anything about async () => realm.write(() => …). The latter seems like a major bug in realmjs. Reading data from a server using async await and then writing it to realm seems like the primary use case of realm!

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 2, 2022

As a short term workaround could you expose a promise await realm.transactionsFlushed. The hook can then await this promise to be resolved before creating the listener.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 2, 2022

I've thought of another workaround which would fix my exact issue but wouldn't attack the root cause.

In the example code, the useObject call is always subscribing to the same entity. Therefore we don't need to actually create another listener to realm, we can just create another listener purely in JS land. Doesn't realm already cache objects eg realm.objectForPrimaryKey('foo', '0') returns the same object? If so why doesn't it also cache realm listeners? Surely having a JS land listener is way less overhead than a realm listener?

@takameyer
Copy link
Contributor

@mfbx9da4 we did some more digging. When the second write transaction starts, any event listeners that haven't finished yet are given time to complete their tasks. In this case, the event listener in useObject runs its course and ends up calling addEventListener, which it is not allowed to do if it is in a write transaction. Adding a sleep(1) to between the two write transactions allows the event listener to run its course before the next write transaction occurs.

We are going to continue to think about a good way to solve this. As a workaround, I would recommend trying to minimise the amount of write transactions that are occurring within asynchronous functions (if there is only one transaction occurring, this seems to work just fine). We might also add a check in the useObject/useQuery hooks for the write transaction, to determine if it's allowed to add a listener or not.

Thanks for taking the time to report and comment on this issue. We will report back when we have made some progress.

@takameyer
Copy link
Contributor

@mfbx9da4 We have come up with a plan on how to make this issue go away, but we want to solve it the "right way" and not just make some bandaid that just works for @realm/react. We will be tracking progress on this in #4391

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 4, 2022

Did you ever get to the bottom of why this only happens in conjunction with FlatList? Or have you been able to reproduce without FlatList?

@takameyer
Copy link
Contributor

takameyer commented Mar 4, 2022

@mfbx9da4 It's not related to anything specific with FlatList. You will get the same crash if you replace FlatList with:

      {fooResults.map(() => {
        return <Message/>;
      })}

It's more to do with re-rendering in conjunction with an async function.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 4, 2022

Ah okay I wasn't able to reproduce with a simple .map(). Good to know, more serious than I thought.

@takameyer
Copy link
Contributor

takameyer commented Mar 4, 2022

One other workaround would be not to use useObject within the rendered component to the FlatList. Just pass the item into the component.
The being said, we know what would fix it and we will get it done sooner than later.

@tomduncalf
Copy link
Contributor

Yeah fundamentally what is happening is something like:

  1. Write 1 starts
  2. Add object
  3. Write 1 ends
  4. Write 2 starts
  5. On the notification thread, the change listener as a result of write 1 fires, which tries to remove the existing listener and add a new one (as we have fetched a new version of the object/collection from the Realm, in order to update the object reference) – this throws, because write 2 has already started and listeners cannot be added in a write transaction.

Whereas with e.g. sleep(1) in between the writes, step 4 and 5 switch places and it succeeds.

So it's a general bug, but exacerbated by the way Realm React works (adding/removing listeners frequently). The plan to fix is it to queue up any "add listener" calls that occur while a transaction is in progress, then drain the queue when that transaction ends.

Thanks for the great bug reports @mfbx9da4, they're really appreciated!

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 4, 2022

My pleasure!

Is there an event one could subscribe to when the active transaction finishes?

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 6, 2022

It's not related to anything specific with FlatList

I have tried to recreate the issue once again without FlatList but to no avail. @takameyer I tried your code sample but never get the error. Try uncommenting this line and commenting out the FlatList just beneath, you'll find the error no longer appears.


@tomduncalf I think a slightly more accurate sequence of events would be:

  1. Write 1 starts
  2. Object added
  3. Write 1 ends
  4. The notification thread, notifies the useQuery listener as a result of write 1, causing FooList and the FlatList to re-render. React starts rendering the flat list items
  5. Write 2 starts
  6. React renders a new component which tries to add a listener in its hook

The sequence of events isn't too important as either way it's clear there is a race condition where listeners can be added inside a transaction. And the same listener error can be observed with useQuery as well as useObject.

What I don’t understand is how any other JS code can run in between the transaction starting and ending. The only way I see this being possible is if realm.write does look something like this but I don't think it does 🤔.

realm.write = (callback) => {
  beginTransaction()
  setTimeout(() => {
    callback()
    endTransaction()
  })
}

If that's the case, why not make realm.write fully synchronous, this would make any concurrency issue impossible.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 6, 2022

Also I put together a quick solution which does properly wait until the active transaction is finished:

function useObject<T>(type: string, primaryKey: string): (T & Realm.Object) | undefined {
  const [object, setObject] = useState<(T & Realm.Object) | undefined>(
    realm.objectForPrimaryKey(type, primaryKey)
  )

  useEffect(() => {
    let isMounted = true
    const listenerCallback: Realm.ObjectChangeCallback = (_, changes) => {
      if (changes.changedProperties.length > 0) {
        setObject(realm.objectForPrimaryKey(type, primaryKey))
      } else if (changes.deleted) {
        setObject(undefined)
      }
    }
    if (object !== undefined) {
      waitForNoActiveTransaction()
        .then(() => {
          if (isMounted) object.addListener(listenerCallback)
        })
        .catch(error => console.error('Failed to add listener', error))
    }
    return () => {
      object?.removeListener(listenerCallback)
      isMounted = false
    }
  }, [object, type, primaryKey])

  return object
}

function waitForNoActiveTransaction() {
  return pollFor(() => !realm.isInTransaction, { attempts: 100, interval: 10 })
}

It would, of course, be much better if there was an event I could subscribe to when the transaction finishes.

@Acetyld
Copy link

Acetyld commented Mar 7, 2022

Is there any solution for this?

@mfbx9da4
Copy link
Author

mfbx9da4 commented Mar 7, 2022

@Acetyld you should be able to drop in replace useQuery and useObject by pasting in these lines which fixes the issue.

That said, it's more of a workaround until there is an official fix from the core team.

takameyer added a commit that referenced this issue Apr 21, 2022
Make use of setImmediate to ensure event listeners are added after
write transactions have completed.
Update useQueryHook tests to give the event loop time to finish and add a listener.
Fix a test in useObject render to allow event listners to fire after writes.
takameyer added a commit that referenced this issue Apr 21, 2022
Make use of setImmediate to ensure event listeners are added after
write transactions have completed.
Update useQueryHook tests to give the event loop time to finish and add a listener.
Fix a test in useObject render to allow event listners to fire after writes.
@takameyer
Copy link
Contributor

@mfbx9da4 A fix is now in review. Sorry for the delay, we were looking over various ways to solve this and finally landed on the correct solution.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Apr 21, 2022

Awesome @takameyer, reading that description I was finally able to reproduce in a less convoluted way

const id = 'foo'
const fooCol = realm.objects<Foo>('foo')
fooCol.addListener(() => {
  // This will be triggered inside a write transaction
  fooCol[0]?.addListener(() => {
    console.log('hey ho')
  })
})

realm.write(() => {
  realm.create<Foo>('foo', { id }, Realm.UpdateMode.Modified)
})
await sleep(0)
realm.write(() => {
  realm.create<Foo>('foo', { id }, Realm.UpdateMode.Modified)
})

As soon as the second write transaction is called, it will flush all event listener events.

So the real issue here is that event listeners callbacks are invoked inside a write transaction. If those event listener callbacks in turn also create listeners we get the error "Cannot create asynchronous query while in a write transaction". By enqueuing the in-turn-listener-creators onto the event loop with setImmediate or setTimeout we ensure the active write transaction completes before adding the listeners.

Isn't the real fix though, to just flush the listener callbacks before starting the write transaction? Or am I missing something?

Assuming the setImmediate is the best solution, since this error is possible without a hook doesn't it make sense to wrap the addListener method with setImmediate rather than just in the hook?


Side note, it's a shame realm fires listeners even though the row hasn't changed.

@takameyer
Copy link
Contributor

Isn't the real fix though, to just flush the listener callbacks before starting the write transaction? Or am I missing something?

We looked into this, but the listeners could potentially start another write transaction, and we could be caught in an endless loop of write transactions.

Assuming the setImmediate is the best solution, since this error is possible without a hook doesn't it make sense to wrap the addListener method with setImmediate rather than just in the hook?

I don't think it's a good idea to do this all the time, since this is classically synchronous. If you are writing code outside of the hook, then it is possible to implement setImmediate yourself as a workaround. I am hesitant to force this paradigm onto all calls to addListener. In the hooks case, it is checking that we are in a write transaction and only in this case putting the addListener onto the event loop.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Apr 22, 2022

This nested addListener issue is so edge case you can't reasonably expect users to expect the issue to occur. Let alone expecting them to workout that setImmediate is the right solution and understand why. 🙂

I would also argue that fundamentally addListener only has asynchronous use cases.

I agree it makes sense to skip setImmediate if it's not in a transaction but the core object.addListener can also do that check.

takameyer added a commit that referenced this issue Apr 25, 2022
Fix for the #4375

Add failing test for listener issue
Make use of setImmediate to ensure event listeners are added after
write transactions have completed.
Update useQueryHook tests to give the event loop time to finish and add a listener.
Fix a test in useObject render to allow event listners to fire after writes.
Only use `setImmediate` if realm is in a write transaction.
Update comments to be more clear.
@takameyer
Copy link
Contributor

@mfbx9da4 I see your point, but it would be a breaking change. There are non-react users of this code that will possibly run into issues if we change this at the core level. In any case, this is fixed for @realm/react.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants