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

Memory leak/recursive loop in 12.0.7 in the types #926

Closed
wolfy1339 opened this issue Nov 20, 2023 · 6 comments · Fixed by #929
Closed

Memory leak/recursive loop in 12.0.7 in the types #926

wolfy1339 opened this issue Nov 20, 2023 · 6 comments · Fixed by #929
Labels
released Status: Needs info Full requirements are not yet known, so implementation should not be started Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only

Comments

@wolfy1339
Copy link
Member

@octokit/webhooks v. 12.0.7 seems to introduce some kind of memory leak/recursive loop. v 12.0.6 still works.

Originally posted by @Uzlopak in probot/probot#1926 (comment)

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented, or is being fixed label Nov 20, 2023
Copy link
Contributor

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@kfcampbell kfcampbell moved this from 🆕 Triage to 🔥 Backlog in 🧰 Octokit Active Nov 20, 2023
@kfcampbell kfcampbell added the Status: Up for grabs Issues that are ready to be worked on by anyone label Nov 20, 2023
@wolfy1339 wolfy1339 added Status: Needs info Full requirements are not yet known, so implementation should not be started typescript Relevant to TypeScript users only labels Nov 20, 2023
@wolfy1339 wolfy1339 changed the title Memory leak/recursive loop in 12.0.7 Memory leak/recursive loop in 12.0.7 in the types Nov 21, 2023
@wolfy1339
Copy link
Member Author

This is the only commit in that release, octokit/webhooks.js@6ead0b7

I don't see any infinite looping happening here, or how there would be one

It crashes the TypeScript compiler in this repo for me. So it's not a runtime, but a build-time problem.

<--- Last few GCs --->

[19282:0x5af5b50]    13766 ms: Scavenge 2024.2 (2077.5) -> 2019.5 (2078.5) MB, 6.1 / 0.0 ms  (average mu = 0.531, current mu = 0.349) allocation failure; 
[19282:0x5af5b50]    13790 ms: Scavenge 2025.1 (2078.5) -> 2021.1 (2080.5) MB, 7.1 / 0.0 ms  (average mu = 0.531, current mu = 0.349) allocation failure; 
[19282:0x5af5b50]    13914 ms: Scavenge 2027.1 (2080.5) -> 2022.9 (2097.5) MB, 110.7 / 0.0 ms  (average mu = 0.531, current mu = 0.349) allocation failure; 


<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
 1: 0xb85bc0 node::Abort() [node]
 2: 0xa94834  [node]
 3: 0xd66d10 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xd670b7 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xf447c5  [node]
 6: 0xf56cad v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 7: 0xf313ae v8::internal::HeapAllocator::AllocateRawWithLightRetrySlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
 8: 0xf32777 v8::internal::HeapAllocator::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
 9: 0xf12cc0 v8::internal::Factory::AllocateRaw(int, v8::internal::AllocationType, v8::internal::AllocationAlignment) [node]
10: 0xf0a734 v8::internal::FactoryBase<v8::internal::Factory>::AllocateRawWithImmortalMap(int, v8::internal::AllocationType, v8::internal::Map, v8::internal::AllocationAlignment) [node]
11: 0xf0c9e8 v8::internal::FactoryBase<v8::internal::Factory>::NewRawOneByteString(int, v8::internal::AllocationType) [node]
12: 0x11ea03d v8::internal::String::SlowFlatten(v8::internal::Isolate*, v8::internal::Handle<v8::internal::ConsString>, v8::internal::AllocationType) [node]
13: 0x11ecc4e v8::internal::String::SlowEquals(v8::internal::Isolate*, v8::internal::Handle<v8::internal::String>, v8::internal::Handle<v8::internal::String>) [node]
14: 0x12fa3e5 v8::internal::Runtime_StringEqual(int, unsigned long*, v8::internal::Isolate*) [node]
15: 0x1705b39  [node]
Aborted (core dumped)

I get this on npm run build. Could be a bug in Node or TypeScript though....

Originally posted by AaronDewes in probot/probot#1926 (comment)

@wolfy1339
Copy link
Member Author

This is the only commit in that release, 6ead0b7

This is only a type change.

It seems that typescript really does not like computing all the events with a TTransformed twice (once for onAny and another for onError)

Does anyone have suggestions on how to precompute, or lazily compute this somehow?

@wolfy1339
Copy link
Member Author

Compiling @octokit/webhooks alone itself is fine. It's when you import it into a big project that things seem to go sideways

@wolfy1339
Copy link
Member Author

I performed a type trace with TypeScript, while increasing the heap size to 12000, and it churned out a trace of all types in the project and how long it took for each one. From that trace, I ran an analysis to transform the trace into human readabable format.

The conclusion is indeed that the changes in @octokit/webhooks 12.0.7 introduced too much recursion for typescript with the default heap size for node

I attached it here if anyone would like to look through it

analysis.txt

Copy link
Contributor

🎉 This issue has been resolved in version 12.0.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Status: Needs info Full requirements are not yet known, so implementation should not be started Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants