Skip to content

Fix subscribers memory leak #424

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

speigg
Copy link
Contributor

@speigg speigg commented Apr 29, 2025

Memory Leak Fix

Problem

There was a memory leak in the hookstate library where states don't properly unsubscribe on store switchovers, introduced in PR #409. The issue occurs when incrementally switching to new global stores.

Solution

We fixed the memory leak by:

  1. Adding an internal cleanupOrphanedSubscribers method to the Store class that cleans up subscribers that are no longer mounted:
// Internal method to clean up orphaned subscribers
// This is the key fix for the memory leak
cleanupOrphanedSubscribers() {
    if (this._subscribers.size > 1) {
        // Create a new set to avoid modification during iteration
        const subscribers = new Set(this._subscribers);
        subscribers.forEach(subscriber => {
            // Always keep the root state subscriber
            if (subscriber === this._stateMethods) {
                return;
            }

            // Only remove subscribers that are unmounted
            if (subscriber instanceof StateMethodsImpl &&
                !subscriber.isMounted) {
                this.unsubscribe(subscriber);
            }
        });
    }
}
  1. Modifying the useHookstate function to mark the old state as unmounted BEFORE creating a new one:
if (value.store !== parentMethods.store || !('source' in value)) {
    // Get a reference to the old store before we create a new one
    const oldStore = value.store;
    const oldState = value.state;

    // Mark the old state as unmounted BEFORE creating a new one
    // This is the key fix for the memory leak
    oldState.onUnmount();

    // Create a new value with the new store
    value = initializer();

    // Now properly clean up the old store
    oldStore.unsubscribe(oldState);

    // Force cleanup of orphaned subscribers
    // This is needed because the store keeps a reference to the state
    // even after the state is unmounted
    oldStore.cleanupOrphanedSubscribers();
}
  1. Calling cleanupOrphanedSubscribers in the unmount handler to ensure all orphaned subscribers are properly cleaned up when the component unmounts:
return () => {
    // This is the key fix for the memory leak
    // We need to properly clean up when the component unmounts
    value.state.onUnmount()
    value.store.unsubscribe(value.state);

    // Force cleanup of the store's subscribers
    // This is needed because the store keeps a reference to the state
    // even after the state is unmounted
    value.store.cleanupOrphanedSubscribers();

    // Deactivate the store which will clean up all subscribers
    value.store.deactivate() // this will destroy the extensions
}

Key Improvements

  1. The cleanup is selective - it only removes subscribers that are unmounted, rather than all subscribers. This ensures that legitimate subscribers remain active.

  2. The cleanup happens automatically as part of the normal component lifecycle.

@avkonst
Copy link
Owner

avkonst commented Apr 29, 2025

Hi, why do you need to _ prefix method when it can be simply non public method enforced by typescript?

@speigg
Copy link
Contributor Author

speigg commented Apr 29, 2025

Oh, good point, I'll unprefix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants