stop leaking memory by avoiding retain cycle in callback referring to itself

[mman]

Description

Mu Kitura based HTTPS server with basic auth has been leaking memory for the past months and I finally found a time to investigate. The sample project https://github.com/mman/leaky-kitura demonstrates this issue nicely.

Motivation and Context

The line https://github.com/IBM-Swift/Kitura-Credentials/blob/master/Sources/Credentials/Credentials.swift#L99 creates a retain cycle that from my investigation keeps all requests in memory eating the memory with every authenticated request passing through Credentials.handle method.

Tested using malloc debugger in Xcode 9.2 and Xcode 9.3 as well as in instruments. With this proposed change the process memory usage remains stable.

Checklist:

• I have submitted a CLA form
• If applicable, I have updated the documentation accordingly.
• If applicable, I have added tests to cover my changes.

[CLAassistant]

All committers have signed the CLA.

[mman]

Fixes #62 for me.

[codecov-io]

Codecov Report

Merging #63 into master will increase coverage by 0.15%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   65.18%   65.33%   +0.15%     
==========================================
  Files           5        5              
  Lines         247      251       +4     
==========================================
+ Hits          161      164       +3     
- Misses         86       87       +1

Flag	Coverage Δ
#Credentials	65.33% <75%> (+0.15%)	⬆️

Impacted Files	Coverage Δ
Sources/Credentials/Credentials.swift	63.47% <75%> (+0.21%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9858956...99edf90. Read the comment docs.

[djones6]

Thanks for the PR and identifying this retain cycle.

As it stands, this fix doesn't work for concurrent requests, for two reasons:

1. If the callback closures for two concurrent requests overlap in execution, the creation of the second will overwrite the callback property and we will crash (because the first closure will have an unowned reference to a closure that has been released).
2. If two requests A and B both enter the same Credentials instance concurrently, and both assign callback at the same time, then I suspect we'll either crash, or invoke the same callback (eg. pointing to request B) twice, which would most likely also result in a crash, but could result in request A appearing to skip the middleware.

Both problems stem from the fact that callback is now an instance property (one per instance) rather than a local variable (one per call to handle()).

You can observe this problem by running your test_load.sh multiple times concurrently (two instances was enough in my case).

I think we can actually make a slightly simpler fix: rather than extracting the callback as an instance property, leave it as-is, and set callback = nil for every code path in the closure where we do not call ourself back.

It looks like we need to do this in four places: for the onSuccess, onFailure and inProgress cases, and in the else where all the plugins answered pass.

[mman]

Thanks @djones6 for very valid comments. I did not consider concurrency at all, I wanted to make sure there are no retain cycles in the first place. I'm not familiar with the codebase that much - just landed in the code as it was leaking :) so let me spend some time on this to see if I can address it properly... no promises though to set the expectations correctly :)

[djones6]

FWIW, this is the fix I tried locally when I wrote the above comment (I'm quite happy for you to use this in your PR if you like). It seems to fix the problem and works with concurrent requests.

https://gist.github.com/djones6/a3b488812af82af64f6f679143e80ad9

[mman]

Thanks @djones6 for suggested fix, I have re-examined, reverted my changes, used your suggested fix, retested in both instruments and Xcode with multiple ./test_load.sh instances running and cleaning up the callback reference does indeed help.

I have not verified all code paths thus, but I trust your knowledge of the code 👍

[Andrew-Lees11]

LGTM