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.

Sign up for GitHub

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

Jump to bottom

Fix retain cycle(s) in encoder implementation #220

Merged
merged 5 commits into from
Nov 23, 2022
Merged

Fix retain cycle(s) in encoder implementation #220

merged 5 commits into from
Nov 23, 2022

Conversation

petrpavlik
Copy link
Contributor

@petrpavlik petrpavlik commented Oct 16, 2022 •
edited
Loading

This PR addresses issue #215 and server as an alternative of PR #216, that somehow got stuck in it's efforts of being merged in.

I've done 2 things to discover the issue and confirm the fix

  • Xcode would show a bunch of memory leaks when using it's memory graph debugger, those are gone after the changes introduced in this PR
  • I've switched https://swiftpack.co, which uses leaf heavily, to using my fork with this fix. The heroku graph bellow shows significantly lower memory consumption and does not seem to be crashing, though I still do do seem to have memory leaks somewhere in the codebase.

Screenshot 2022-10-11 at 8 09 09

Screenshot 2022-10-09 at 18 38 05

Screenshot 2022-10-09 at 18 35 00

Screenshot 2022-10-09 at 18 36 58

Screenshot 2022-10-09 at 18 32 46

Screenshot 2022-10-09 at 18 35 33

@petrpavlik petrpavlik changed the title try to fix a memory leak Fix retain cycle(s) in encoder implementation Oct 16, 2022
0xTim
0xTim requested changes Nov 22, 2022
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change to make then we're good

Sources/Leaf/LeafEncoder.swift
@@ -136,8 +136,9 @@ extension LeafEncoder {
}

private final class KeyedContainerImpl<Key>: KeyedEncodingContainerProtocol, LeafEncodingResolvable where Key: CodingKey {
private let encoder: EncoderImpl
private weak var encoder: EncoderImpl!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this optional since we unwrap it anyway. That provides a fatal error with a message rather than just crashing (and won't complain when we roll out swiftlint)

@0xTim 0xTim added the semver-patch Internal changes only label Nov 22, 2022
@0xTim 0xTim linked an issue Nov 22, 2022 that may be closed by this pull request
Memory Leak on View Render with Encodable Struct/Enum #215
Closed
@0xTim 0xTim mentioned this pull request Nov 22, 2022
Closed
@betaphi betaphi mentioned this pull request Nov 23, 2022
Closed
0xTim
0xTim reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested changes so I can merge them in

Sources/Leaf/LeafEncoder.swift Show resolved Hide resolved
Sources/Leaf/LeafEncoder.swift Show resolved Hide resolved
0xTim
0xTim reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why you don't code on your phone

Sources/Leaf/LeafEncoder.swift Outdated Show resolved Hide resolved
Sources/Leaf/LeafEncoder.swift Outdated Show resolved Hide resolved
@0xTim
Copy link
Member

0xTim commented Nov 23, 2022

Will merge as is and we can fix the IUOs in a future PR. Since this has been running with a real site I'm happy to merge

@0xTim 0xTim merged commit 79fbcfb into vapor:main Nov 23, 2022
@VaporBot
Copy link

These changes are now available in 4.2.3

@bartjacobs
Copy link

@petrpavlik @0xTim Thanks so much for fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xTim 0xTim 0xTim approved these changes

Assignees
No one assigned
Labels
semver-patch Internal changes only
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory Leak on View Render with Encodable Struct/Enum
4 participants
@petrpavlik @0xTim @VaporBot @bartjacobs