-
Notifications
You must be signed in to change notification settings - Fork 892
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
Fix low-memory issue and lower tier platforms with no sysinfo #2779
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
In rust-lang#2774 we have seen hangs during installation for some low tier platforms for whom the 32M default if sysinfo doesn't work causes problems. This change *exposes* that problem in a failing test. Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
57455f6
to
03be992
Compare
In incremental file processing we need to correctly account for the final chunk being released, otherwise we end up holding a chunk for the duration of the rest of the component for every incremental file in the tarball. This is what caused the issue surfaced in the previous commit, and thus this commit fixes rust-lang#2774 Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
03be992
to
1e4de78
Compare
Scope trace added, hope that's good now. |
heh check the timeline... I had already done so :) |
@kinnison mentioned the potential for a release in order to deal with this. Is a 1.24.3 release planned? If so, I'd be curious if there are any other things blocking it. |
@chrisduerr Once #2782 is done I think a new release is plausible. Assuming Robert has time to review and merge it soon, I'll aim to stage a beta release early in the week. |
Lower memory (where 32M might be the bare minimum we can offer the unpacker) or lower-tier platforms which lack sysinfo support (where we default to 32M of unpack RAM) were hanging during install. Turns out this is because we were failing to account for the release of the last chunk in an incremental file unpack. This wasn't a memory leak per-se, but it did mean that the accounting system refused to let the component proceed because it didn't think there was a free chunk to use.
This pair of commits first introduces a test change which demonstrates the issue and then introduces a fix.
If this is merged, we should do a release in order to deal with #2774 asap.