WeakMap error at Repeater.return

[evelant]

Running into an issue with repeaterjs 3.0.4, my repeater is crashing when I try to stop it by calling return.

 Error: WeakMap error
      at Repeater.return 
    .....my app stack

Not a lot to go on here. I'm calling return() to stop a repeater and create a new one when some input conditions change. I'm wrapping a react-native-firebase firestore onSnapshot listener in a repeater and my query can change so I need to stop and re-create the listener and repeater when that happens.

[evelant]

Hmm ok it looks like it's happening here: https://github.com/repeaterjs/repeater/blob/219a0c8faf2c2768d234ecfe8dd21d455a4a98fe/packages/repeater/src/repeater.ts#L508

but I'm not sure why yet

[brainkim]

@AndrewMorsillo I’m sorry to hear that! I want to investigate this ASAP as in tonight, tomorrow, whenever you get back to me. If you have a reproduction I would be eternally grateful! But maybe there’s something about your environment where weakmaps are just broken which would make me rethink them as a technology. Alternatively, if you don’t mind me poking around in your repo I’d be glad to help in that way too. Let’s get this resolved.

[brainkim]

To be clear, I’ve reviewed the code and I don’t think there’s anything obviously wrong with the way I’m using WeakMaps. If you’re seeing that error, something terrible and unforeseen has happened and it could be as serious as a use-after-free security vulnerability in whatever environment you’re working with. Again, I am super interested in more information about this bug, and would gladly spend a day or more debugging why this is happening because it has larger implications than just repeaters. No pressure though! 😇😬

[evelant]

I'm on react-native 0.63.4 with @react-native-community/cli 4.14.0. I had to add "@babel/plugin-proposal-async-generator-functions" to babel.config.js to get repeaters running on react-native.

I don't see anything obviously wrong with the repeaterjs code here either (but obviously you're way more familiar with it than me). Very strange!

Unfortunately I don't have any time available to dig into this at the moment. My repo is not open source so I don't have a reproduction to share at the moment. I've worked around this by removing my use of repeaters for now. I didn't really need to use repeaters I just wanted to because I thought it was an elegant API =)

Here's a sample of the offending function

/**
* Listen for changes to a firestore query and emit them into a repeaterjs repeater
*/
export function listenToFirestoreQuery<T extends { [key: string]: any }>(
    collectionName: string,
    [path, op, value]: [string | FirebaseFirestoreTypes.FieldPath, FirebaseFirestoreTypes.WhereFilterOp, any]
) {
    const snapshots = new Repeater<FirestoreQueryChangeType<T>>(async (push, stop) => {
        const stopSnapshotListener = firebase
            .firestore()
            .collection(collectionName)
            .where(path, op, value)
            .onSnapshot(
                (snapshot) => {
                    snapshot.docChanges().forEach((docChange) => {
                        log.info(`${collectionName} got change ${docChange.type}`)
                        push({
                            changeType: docChange.type,
                            //No types from firestore, cast here
                            doc: docChange.doc.data() as T,
                            hasPendingWrites: docChange.doc.metadata.hasPendingWrites,
                            fromCache: docChange.doc.metadata.fromCache,
                        })
                    })

                },
                (err) => {
                    log.error(`Error in onSnapshot for ${collectionName}`, { error: err }, { reportAsError: true })
                    stopSnapshotListener()
                    stop(new Error(`firestore onSnapshot error ${err.name} ${err.message}`))
                }
            )
            await stop
            stopSnapshotListener()
    })

    return snapshots

Then where I was consuming this and the error was being raised the pattern was like this. The WeakMap error was thrown when this function got called for a second time and Repeater.return was called to tear down the first invocation.

let stopListener: (() => void) | null = null
async function listenToIds(ids: string[]){
     stopListener?.() //This is where repeaterjs threw the WeapMap error when this function was re-run with new ids

     const changes = listenToFirestoreQuery<MyMSTModelSnapshotType>(
                "myCollectionName",
                ["userId", "in", ids])

     stopListener = changes.return

     for await (const change of changes){
             ...process changes as they come in
     }
}

Do you see anything wrong with this pattern? It seemed like a pretty "standard" way to wrap a callback based API in a repeater but I may have done something wrong without realizing it.

[brainkim]

@AndrewMorsillo Ah I see what happened. False alarm! changes.return is a method and when you assigned it to stopListener the receiver was lost, hence the WeakMap error. If you’re not sure what I’m talking about this article might be a good resource.

If you change stopListener = changes.return to stopListener = changes.return.bind(changes), it should work. This is my fault for not writing a clearer error message for when a method is passed around without its context like this, and if you would like, I’d like the leave this issue open until I get around to creating a better error message.

[evelant]

@brainkim ah I see. I almost never use classes/this in my code so the this binding issue hadn't occurred to me at all!

I don't see any case where you wouldn't want those methods bound to the current repeater instance. Instead of throwing an error why not always bind methods to the correct instance? You could use class property initializers like so:

return = (
    value?: PromiseLike<TReturn> | TReturn,
  ): Promise<IteratorResult<T, TReturn>>  => {
    //.... no longer need to worry about `this`, it's always properly bound to this instance
}

Maybe I'm missing something but I think the pattern that I was using would be pretty common. It would be a bit unwieldy to always need to explicitly bind methods to instances in those cases.

[brainkim]

@AndrewMorsillo The problem with eagerly binding methods to class instances is that you’re essentially creating a new function in memory for every method for every instance, versus having methods defined once and shared via the JavaScript prototype chain. This can represent a substantial increase in memory usage, and the most famous example of this issue in the wild is early days React moving away from “method autobinding”.

In 95% of situations, this is usually set correctly, so it’s just wasted memory. Furthermore, one of the design goals of repeaters has always been to be an idiomatic, almost native kind of class like promises, and native classes do not autobind methods either. For instance, attempting to use Promise.prototype.then() without the receiver will throw a TypeError.

const p = new Promise(() => {});
const fn = p.then;
fn(); // this line throws an error

For these reasons, I think a better error message and using bind or calling the method in a closure is the right approach for developers. Let me know if you think differently and we can work through it 😊

[evelant]

@brainkim Thanks for the explanation, that makes sense. I think a better error message would be a great solution given the design goals.