Combine version 1.0 (I think, be nice if the version # of the book was easy to find inside the book in iBooks, perhaps on the copyright page?) Chapter 8 “wrapping a callback function as a future” section, comment number positions for this section seem off:
Hey! This didn’t make it into the 1.0.1 release but was fixed and will be updated in the next release. Thanks !
Hey Peter 
No need to get agitated. I can understand the frustration, and trust me that we do our best to go through ALL reader feedback. This specific feedback might’ve slipped through the cracks here, but we’ll make sure it’s fixed in the upcoming release. Thank you for letting us know!
Hi @knight556, sorry about that and thanks for reporting! This will be fixed in the next release.
Thank you! Came here to say this, I was going nuts until I opened up the final project.
Chapter 8 project; page 197 on v1.0.2 PDF shows:
let newPhotos = photos.selectedPhotos
newPhotos
.map { [unowned self] newImage in
// 1
return self.images.value + [newImage]
}
// 2
.assign(to: \.value, on: images)
// 3
.store(in: &subscriptions)
That store(in:) line is a problem because it causes a memory leak. Every time you click the + button you get another subscription added to the subscriptions Set:
▿ 1 element
▿ 0 : <AnyCancellable: 0x6000025d47e0>▿ 2 elements
▿ 0 : <AnyCancellable: 0x6000025e86e0>
▿ 1 : <AnyCancellable: 0x6000025d47e0>▿ 3 elements
▿ 0 : <AnyCancellable: 0x6000025e86e0>
▿ 1 : <AnyCancellable: 0x6000025d47e0>
▿ 2 : <AnyCancellable: 0x6000025ed840>4 elements
▿ 0 : <AnyCancellable: 0x6000025ed6a0>
▿ 1 : <AnyCancellable: 0x6000025ed840>
▿ 2 : <AnyCancellable: 0x6000025d47e0>
▿ 3 : <AnyCancellable: 0x6000025e86e0>▿ 5 elements
▿ 0 : <AnyCancellable: 0x6000025ed6a0>
▿ 1 : <AnyCancellable: 0x6000025ed840>
▿ 2 : <AnyCancellable: 0x6000025d47e0>
▿ 3 : <AnyCancellable: 0x60000250c860>
▿ 4 : <AnyCancellable: 0x6000025e86e0>▿ 6 elements
▿ 0 : <AnyCancellable: 0x6000025ed6a0>
▿ 1 : <AnyCancellable: 0x6000025ed840>
▿ 2 : <AnyCancellable: 0x6000025d47e0>
▿ 3 : <AnyCancellable: 0x60000250c860>
▿ 4 : <AnyCancellable: 0x600002509820>
▿ 5 : <AnyCancellable: 0x6000025e86e0>▿ 7 elements
▿ 0 : <AnyCancellable: 0x6000025ed6a0>
▿ 1 : <AnyCancellable: 0x6000025ee440>
▿ 2 : <AnyCancellable: 0x6000025d47e0>
▿ 3 : <AnyCancellable: 0x6000025e86e0>
▿ 4 : <AnyCancellable: 0x60000250c860>
▿ 5 : <AnyCancellable: 0x600002509820>
▿ 6 : <AnyCancellable: 0x6000025ed840>
sending the .finished event on the publisher does not cause the subscription to be removed from the subscriptions Set.
P.S. It would be helpful to have the version number in the text itself on the first or second page of the PDF of the book. I’m hoping the file name is correct.
I addressed this by adding the following instance variable to MainViewController:
private var newPhotosSubscription: AnyCancellable?
and then changing the quoted code to:
self.newPhotosSubscription = newPhotos
.map { [unowned self] newImage in
self.images.value + [newImage]
}
.assign(to: \.value, on: images)
which may have other issues, but at least no longer leaks memory
Same issue exists in the next section that talks about implementing the actionSave() method. The AnyCancellable is again stored in subscriptions Set instance variable and remains there until the app is quit.
Repeated saves with a breakpoint at the start of actionSave printing the subscriptions Set show contents growing by one each time:
1 element
▿ 0 : <AnyCancellable: 0x600000b78a40>
▿ 2 elements
▿ 0 : <AnyCancellable: 0x600000ba00e0>
▿ 1 : <AnyCancellable: 0x600000b78a40>
▿ 3 elements
▿ 0 : <AnyCancellable: 0x600000ba00e0>
▿ 1 : <AnyCancellable: 0x600000b78a40>
▿ 2 : <AnyCancellable: 0x600000ba2300>
Same fix. A better pattern would be nice. Having instance variables for each of these is going to get annoying. I tried to find a clean way to have the receiveCompletion handler remove the AnyCancellable from the set but didn’t come up with an elegant way to do that, yet 
Ironically the next section after that is about Memory Management but doesn’t address this issue at all.
Ah, the showMessage is another of this is same situation.
I got tired of the proliferation of instance variables and so came up with another approach: Improving on the common AnyCancellable .store(…) pattern. – Geek And Dad's Blog
UPDATE: → Interestingly, when I add the second subscription on newPhotos this strategy seems to have trouble. I don’t understand why exactly yet. Possibly one is sending a completed or cancel that’s impacting the other?
Hey there @dad! Thanks for the comment. To my understanding there’s no leak here.
The reference to the subscription itself is stored in the Set because a value can’t “remove itself” from a Collection, but the subscription it points to should be dead / inactive.
As soon as the set is cleared, all of these “old” references are cleared and any subscriptions that are still active (e.g. not canceled), will be canceled.
Hope this clears things up!
Hey @freak4pc, thanks for the reply. Looks to me like the AnyCancellable objects accumulate in the Set forever because the Set is an instance variable on the MainViewController which will never be released while the app is running. The number of items in the Set will grow without bound as the user interacts with the app. That’s a “leak” by any definition I’ve ever heard used.
I don’t know how much data is behind the AnyCancellable (haven’t dug into that), so not sure how long it’ll take for the app to run out of memory. One question is if it holds onto the publisher or not.
I suppose one could write an automated UI Test to open and close the add photos view over and over and find out.
I realize the value can’t remove itself, which is why the subscription pipeline needs to do it. I provided a link to a blog post where I detailed my strategy for doing that (too long for a comment here).
This turns out to reveal some interesting issues though. I’m still tracking down what’s going on. For some reason removing the AnyCancellable for the image adding subscription is cancelling not only that subscription but also the subscription that the later step added to update the UI after a 2 second delay. That shouldn’t be the case unless .share() is working less usefully than it seems like it should. In my simple test case this doesn’t happen, only in this app. There appear to be interactions between which thread things are happening on and the use of delay.
That’s a “leak” by any definition I’ve ever heard used.
In my book it’s not really, depending on the scenario. If your MainViewController stores an array of 5 integers, is that a memory leak? There are only references to “dead” subscriptions, not actual work. A “leak” would also mean memory is not released due to some retain cycle or unexpected circumstances.
In this case, though, it is probably unneeded (but not harmful since there’s no heavy work done) to have several subscriptions. I’ll take an extra look to make sure, but the subscription should probably be set-up only once on viewDidLoad, and not every time the button is tapped. cc/ @icanzilb
I agree with “depending on the scenario”. If it was a fixed number of something small, as your example of “5 integers” suggested, I’d totally agree. I’d call that a “fixed cost” or “overhead” and not a “leak”.
As currently implemented, it’s an ever growing number of objects in memory that increases based on repeated execution of some code (triggered by user action in this case) and thus a memory leak. It is exactly a case of “memory is not released,” as you say. A retain cycle is one common cause of memory leaks, but certainly not the only one. “Failing to dispose of something that should be” is another. That’s a less common one now that we have ARC, but still possible, as this code demonstrates.
Now, I’ll be the first to agree that this is not very likely to be an issue in this app (unless it retains the whole publisher memory tree or something nasty like that). I mean, how many times is a user going to hit the add photos button (or save etc), realistically?
The reason I’m bothering to push on this, and care, is because this is likely to be an often-read book and teaching good memory management habits is important in that context. For a quick hack-test then there is no reason to care.
Imagine a scenario where someone wrote a photo processing app that processed a large batch of photos and re-used the PhotoWriter.swift file from this project. If they copied the code in actionSave()from MainViewController.swift which uses PhotoWriter to save an image and which follows the same pattern of saving the AnyCancellable into the subscriptions Set, then this memory leak is a lot more likely to be a significant problem for them (particularly if the number of photos processed is large).
Your idea of setting up the subscription once is interesting. Not sure how it works since the publisher goes away when the PhotosViewController is dismissed (presumably). Likewise for the other places where the AnyCancellable is saved into the subscriptions Set (PhotoWriter which uses a transitory Future publisher, etc).
Thanks again for the reply. Onward!
Chapter 5 (v1.0.2) - switchToLatest()
It is suggested that subscriptions are cancelled on switching to the next, and in the example (p211) we don’t bother completing publisher1 and 2 though we do complete publisher3. It lead me to believe that those publishers were cancelled, but we could happily switch back to the other publishers and get future values (but not the ones that were sent while attached to the other stream)
let publisherA = PassthroughSubject<Int, Never>()
let publisherB = PassthroughSubject<Int, Never>()
let publishers = PassthroughSubject<PassthroughSubject<Int, Never>, Never>()
publishers
.switchToLatest()
.sink(receiveCompletion: { print("Recieved completion", $0) },
receiveValue: { print("Received value", $0) })
publisherA.send(0)
publishers.send(publisherA)
publisherA.send(1)
publishers.send(publisherB)
publisherA.send(2)
publisherB.send(3)
publishers.send(publisherA)
publisherA.send(4)
// Output is 1 3 4
publisherA.send(completion: .finished)
publisherB.send(completion: .finished)
publishers.send(completion: .finished)
It isn’t hard to see how suggested corrections are slipping through the cracks. Perhaps, an errata that organized changes in an ordered list could be useful. It is difficult to find corrections in this forum.
That’s not really the case. We are following this forum and any additional feedback, but usually don’t fix individual feedback but accumulate it for additional releases. If time will allow, we’ll release additional releases in between but it’s not something we can guarantee at the moment.
Thanks ! And please keep submitting feedback.
Looking at the text I wouldn’t say that the book misleads the reader - it does say that storing subscriptions in subscriptions ties the subscription life to the view controller:
subscriptionsis the collection where you will store any UI subscriptions tied to the lifecycle of the current view controller. When you bind your UI controls tying those subscriptions to the lifecycle of the current view controller is usually what you need. This way, in case the view controller is popped out of the navigation stack or dismissed otherwise, all UI subscriptions will be canceled right away.
I don’t really appreciate you reposting the code without the explanation what it does because this way you mislead folks who’ll read your blog. But that issue aside, if you want to handle your subscriptions differently (e.g. NOT tie their disposal to the disposal of your view controller like in the chapter) you’re welcome to do that - the approach you posted on your blog seems 
to me.
Thanks for the clarification @icanzilb
@dad — I agree with Marin that your blog post is unfair/misleasinf because your definition of a memory leak is wrong - a leak would be when you accidentally leave some dead memory beyond the scope of another object in a way that cant be reclaimed. In here - We’re simply tying the subscription context to the view context which is a common and useful pattern.
In this specific case this is a “root” view controller which means these subscriptions will be accumulated, indeed, since the VC itself is never deallocated. In most common scenarios, though, these VCs are pushed and popped / presented and dismissed constantly, so tying these subscriptions to the view makes absolute sense.
Finally, don’t forget that book projects are not production apps. They are used to demonstrate specific points, ideas and concepts - and not an entire app with all of its production constraints.
Regardless, thank you again for the feedback! We might add a note about this in the next edition, but it’s definitely not a critical issue from our perspective as a book team and that behavior is planned in that specific context.
I can see your point - there is still a reference to memory and you could write code to dispose of the memory (hey! What do you know, that’s exactly what my blog post shows how to do!) and so by a strict definition of “memory leak”, sure, it’s not a “leak”. But it will be a memory leak if some new programmer copies the pattern in their root view controller (like in this simple example) and doesn’t understand that they are accumulating memory over time until their app crashes.
Unfortunately, the phrase “memory leak” seems to have triggered a bit of a defense reaction and overshadowed my point which was feedback that the book would be better if it pointed this issue out for less experienced programmers and taught readers a pattern to use in that case.
Because of the reaction my comments got here it seemed like I needed to write a blog post to point out the issue and provide at least one strategy myself - so I did.
In my blog post I specifically said,
“The problem with this pattern in this particular case, and other similar situations, is that the any
AnyCancellableis never removed from the subscriptionsSetand so it’s a memory leak. Sometimes this doesn’t matter - when the enclosing scope is going to go away and the code is only called once relatively soon before existing scope cleans up memory.”
That all seems to be very specifically explaining what the issue is, when it’s an issue, and when it isn’t. Doesn’t seem “unfair/misleading” to me, but I’m sorry you feel that way; it wasn’t my intent that anyone should feel insulted or attacked.
In my blog post I made a point of linking to your book and saying “It’s a good book on Combine”, which I thought you would feel was of benefit to you; clearly you do not and so I have removed reference to your book from the post.
I have also heard your feedback and modified the language in the post to remove the phrase “memory leak”, since it seems to cause problems, and I tried to clarify the language in the section quoted above.
So, thanks for the feedback. Edits were made.
Chapter 8: Presenting a view controller as a future
The
.handleEvents(receiveCancel: {
self.dismiss(animated: true)
})
part when displaying an alertController seems to be unnecessary as it will dismiss itself.
Chapter 3: Transforming operators
You create the Chatter object with the names of Charlotte and James, but they’re not really used in the rest of the code. The output is then showed as
Charlotte wrote: Hi, I'm Charlotte!
James wrote: Hi, I'm James!
but the following sink code will not produce that output:
chat
.sink(receiveValue: { print($0.message.value) })»
Unless I’m missing something this is exactly the output the chapter has. If you run the final playground and remove the flatMap from the flatMap example, this is the output you’ll see:
——— Example of: flatMap ———
Hi, I'm Charlotte!
Hi, I'm James!
Hey guys, what are you up to?
What is exactly missing for you?