Memory leak in error path

[Domino9697]

Describe the bug

Hello everyone !

Thank you for this library ! We are actively using it at the present moment and I would need your help regarding a memory leak we discovered.

Description

We are currently facing an issue where the memory consumption of our docker containers are increasing over time.
We are hosting a sveltekit application and a backend exposing a graphql API. The application uses Houdini as the fetch client. After some investigation, I can say the issue is coming from the Houdini library.
From what I can see, Houdini stores are not removed from memory if an error is thrown right after a fetch call or if the fetch call itself throws an error.

I created a reproduction repo here based on the pokedex 😄

Here is the important part where we artifically throw an error after calling fetch that creates the memory leak:

export async function load(event) {
    const { Info } = await load_Info({
        event,
        variables: {
            id: 2
        })

    // This throws an error
    error(500)

    return {
        Info
    }
}

Steps to reproduce

See the repro repo Readme here

Expected result

The memory usage should not increase over time.
The number of houdini document stores should not increase over time and should be limited to just the current active stores when the snaphsot was taken.

Actual result

The memory usage increases over time.
The number of houdini document stores increases over time. I could get thousands of stores in a few minutes.

Investigation comments

When looking at why this is happening, I saw that an active subscription is kept in memory if an error is thrown after calling fetch. I looked at the Houdini source code and found that the issue is coming from here.
When we call fetch, we call the BaseStore setup function which subscribed to an observer... And I see that we do unsubscribe from this when a unsubscribe call is made to the Houdini store. However, this might never happen especially in the scenario mentioned above.

This is not an issue in the nominal path because the library expects users to return the store in svelte components and then subscribe to it. However, if the fetch itself throws or if a throw happens after calling fetch, the store is not subscribed to and unsubscribe is not called.

To verify this, we can add a get call right after fetch is called and look at the results.

export async function load(event) {
	const { Info } = await load_Info({
		event,
		variables: {
			id: 2
		}
	})

	get(Info)

    // This throws an error
	error(500)

	return {
		Info
	}
}

By doing this, we don't see the leak anymore.

Fix for now

On our side we can add a get call right after the fetch call to avoid the memory leak. However, I think this should be fixed in the library itself as this could be a serious issue for a lot of users using manual loading or the throwOnError option.

Since we are getting these issues because we throw when a graphql error is returned, we could use the following hack for now:

export async function load(event) {
    const Info = new InfoStore()
    await Info.fetch({
        event,
        variables: {
            id: 2
    }).catch((err) => {
        get(Info)

        throw err
    })


    return {
        Info
    }
}

I have no idea if this would break some functionalities but after some testing, it seems to work fine.

Reproduction

https://github.com/Domino9697/houdini-memory-leak

[AlecAivazis]

Okay, thanks to your thorough investigation i think i was able to get a fix pretty quickly. I'm not sure how to test it using the reproduction since i can't navigate client-side once the error has been thrown - am i missing something? Anyway, you should be able to copy the fixes into your local $houdini directory and check if it works.

[TeyKey1]

I think I might be facing a similar issue, also experiencing a memory leak in production although I get the leak on successful requests as well. Not quite sure why yet.

I'm using Houdini on a SvelteKit endpoint as follows to fetch graphql data and modify it before using it in SSR/CSR:

export const POST: RequestHandler = async function (event) {
        const { params, fetch } = event;
	const id = parseArticleSlugId(params.slug);
	const category = params.category.replaceAll('+', ' '); // Deslugify category for use in API query
	const language_id = LANG_MAPPER.get(params.lang)!;

        // Create Houdini store
	const articleStore = new NewsArticleContentStore();

        // Blocking fetch as result data is being used/modified further below
	const result = await articleStore.fetch({
		event,
		variables: { id, category, language_id },
		blocking: true
	});

	if (result.errors) {
		throw new Error(JSON.stringify(result.errors));
	}

	if (result.data!.news_content.length !== 1) {
		error(404);
	}

	//This function just makes a plain fetch call unrelated to houdini and appends the result to houdini's result data 
	(result.data!.news_content[0].content as string) = await insertImageCaptions(
		fetch,
		result.data!.news_content[0].content,
		language_id
	);

	return json(result.data!, {
		headers: new Headers({
			'Cache-Control': 'max-age=120'
		})
	});
};

This code seems to leak the NewsArticleContentStore regardless if an error is thrown inside the endpoint or not. Not quite sure why this leaks though, as store.fetch() calls get() on return. Maybe I'm doing things I'm not supposed to in the above snippet.

@AlecAivazis I've just loaded in your fix and it seems to work for my case. NewsArticleContentStore is properly cleaned up now.