changed unowned to weak for unsafe situations #9
Conversation
extract method tryToEstablishConnection() from Session
| // Suspend the timer to prevent calling completion two times | ||
| self.timeoutSource!.suspend() | ||
| self?.timeoutSource!.suspend() |
There was a problem hiding this comment.
Instead of having an optional everywhere, can you do something like this?
socketSource.setEventHandler { [weak self] in
guard let command = self else {
return
}
// Suspend the timer to prevent calling completion two times
command.timeoutSource!.suspend()
// [...]
}There was a problem hiding this comment.
Sure, or I can call it strongSelf, that is quite common as well. Take your pick!
There was a problem hiding this comment.
I don't mind the name! I usually use strongSelf, so I'm happy with that 😄
| @@ -54,11 +54,15 @@ public class SSHCommand<T: RawLibrary>: SSHChannel<T> { | |||
| socketSource.cancel() | |||
| } | |||
|
|
|||
| self.queue.async { | |||
| super.close() | |||
| self.queue.async { [weak self] in | |||
There was a problem hiding this comment.
I think self should be retained by the block here, and in all the other places where you've added it.
There was a problem hiding this comment.
Well, that would be great but causes retain cycles and memory leaks. Here is some explanation:
http://katalisha.com/2016/01/22/ARC-Swift-closures-and-weak-self.html
http://www.thomashanning.com/retain-cycles-weak-unowned-swift/
There was a problem hiding this comment.
I'm aware that it's a retain cycle but eventually the block will be executed and the strong reference will disappear. This allow you do to something like this:
// [...]
channel.disconnect {
print("Disconnected")
}
channel = nilWith you change, channel would be released right away without waiting for the disconnection to complete.
|
I'll create a PR for only the unowned, and will do a little research on the unspecified strong references in closures. I'm not entirely sure that the self will ever get released, even after the block got executed. I'll get back to you on that... |
|
I'm going to merge the other one, thanks! |
Using unowned self in async calls or closures can cause crashes or memory leaks. This pull requests fixes most of those unowned self to use weak self, which is much safer in most situations. Hope it helps!