acacode / swagger-typescript-api

Generate the API Client for Fetch or Axios from an OpenAPI Specification
MIT License
3.08k stars 338 forks source link

Memory Leak Due to Response Cloning #779

Open maiky-apex opened 2 weeks ago

maiky-apex commented 2 weeks ago

Description

We have identified a memory leak in our application that appears to be related to the swagger-typescript-api package. After thorough investigation, we traced the issue to a change introduced in this commit, which was merged on September 5, 2024.

Problem

In the commit referenced above, the response of the auto-generated customFetch function is cloned before being returned. This clone operation is causing a memory leak in our application.

Observation

Without the clone operation, the memory leak does not occur. The memory usage increases steadily when the clone method is used, indicating a potential problem with how the response is handled.

Steps to Reproduce

Use the swagger-typescript-api package version that includes the commit ac9988579a2a425d73a6fe59b6aa336287057159. Generate API client code using swagger-typescript-api. Make API requests using the generated client. Observe memory usage over time with and without the response being cloned.

Expected Behavior

The response should be returned without causing a memory leak, and the memory usage should remain stable.

Actual Behavior

The memory usage increases steadily, leading to a memory leak when the response is cloned.

Additional Context

Here's a snippet of the auto-generated code that causes the issue:

    return this.customFetch(`${baseUrl || this.baseUrl || ''}${path}${queryString ? `?${queryString}` : ''}`, {
      ...requestParams,
      headers: {
        ...(requestParams.headers || {}),
        ...(type && type !== ContentType.FormData ? { 'Content-Type': type } : {}),
      },
      signal: (cancelToken ? this.createAbortSignal(cancelToken) : requestParams.signal) || null,
      body: typeof body === 'undefined' || body === null ? null : payloadFormatter(body),
    }).then(async (response) => {
      const r = response.clone() as HttpResponse<T, E>;
      r.data = null as unknown as T;
      r.error = null as unknown as E;

      const data = !responseFormat
        ? r
        : await response[responseFormat]()
            .then((data) => {
              if (r.ok) {
                r.data = data;
              } else {
                r.error = data;
              }
              return r;
            })
            .catch((e) => {
              r.error = e;
              return r;
            });

      if (cancelToken) {
        this.abortControllers.delete(cancelToken);
      }

      if (!response.ok) throw data;
      return data;
    });
  };
}
smorimoto commented 1 week ago

@maiky-apex This seems correct, but I don't have enough time to work on this, so could you raise a PR?

maiky-apex commented 1 week ago

@smorimoto Thank you for the quick response. I understand the importance of the response cloning to address the issue mentioned in PR #670 related to node-fetch issue #533.

While I have confirmed that removing the clone operation resolves the memory leak, I don't want to remove it outright without considering the implications for the previously addressed issue.

Unfortunately, I also don't have enough time at the moment to work on a proper solution that addresses both issues. I will try to work on it soon, but in the meantime, if someone else sees this comment and has a potential solution, please feel free to open a PR.

smorimoto commented 1 week ago

Thanks a lot @maiky-apex

smorimoto commented 1 week ago

@maiky-apex @depsimon I don't know how different the node-fetch implementation is from the standard fetch implementation, but keeping in mind that we are now using native fetch, we may also need to verify that the commit is really necessary.