Skip to content

Commit

Permalink
Fix memory leaks in Promise whenAll
Browse files Browse the repository at this point in the history
  • Loading branch information
AnthonyMDev committed Feb 13, 2020
1 parent de705bc commit 2da9049
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 2 deletions.
7 changes: 6 additions & 1 deletion Sources/Apollo/Promise.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import Dispatch
func whenAll<Value>(_ promises: [Promise<Value>], notifyOn queue: DispatchQueue = .global()) -> Promise<[Value]> {
return Promise { (fulfill, reject) in
let group = DispatchGroup()
var rejected = false

for promise in promises {
group.enter()
Expand All @@ -11,11 +12,15 @@ func whenAll<Value>(_ promises: [Promise<Value>], notifyOn queue: DispatchQueue
group.leave()
}.catch { error in
reject(error)
rejected = true
group.leave()
}
}

group.notify(queue: queue) {
fulfill(promises.map { $0.result!.value! })
if !rejected {
fulfill(promises.map { $0.result!.value! })
}

This comment has been minimized.

Copy link
@RolandasRazma

RolandasRazma Feb 14, 2020

Contributor

else { ?

what about reject? doesn't that need to be called?

This comment has been minimized.

Copy link
@designatednerd

designatednerd Feb 14, 2020

Contributor

reject is already called in the catch block for the promise(s) which failed.

This comment has been minimized.

Copy link
@RolandasRazma

RolandasRazma Feb 14, 2020

Contributor

but wouldn't it be called multiple times if multiple fail where fulfill only once?

This comment has been minimized.

Copy link
@designatednerd

designatednerd Feb 14, 2020

Contributor

Yes, although I think @AnthonyMDev was trying to just concentrate on the one problem here rather than fixing all the problems with our promises implementation. Would love to see another PR addressing this one!

This comment has been minimized.

Copy link
@RolandasRazma

RolandasRazma Feb 14, 2020

Contributor

fair enough, especially as this is already merged :)

}
}
}
Expand Down
7 changes: 6 additions & 1 deletion Sources/Apollo/ResultOrPromise.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func whenAll<Value>(_ resultsOrPromises: [ResultOrPromise<Value>], notifyOn queu

return .promise(Promise { (fulfill, reject) in
let group = DispatchGroup()
var rejected = false

for resultOrPromise in resultsOrPromises {
group.enter()
Expand All @@ -27,11 +28,15 @@ func whenAll<Value>(_ resultsOrPromises: [ResultOrPromise<Value>], notifyOn queu
group.leave()
}.catch { error in
reject(error)
rejected = true
group.leave()
}
}

group.notify(queue: queue) {
fulfill(resultsOrPromises.map { $0.result!.value! })
if !rejected {
fulfill(resultsOrPromises.map { $0.result!.value! })
}

This comment has been minimized.

Copy link
@RolandasRazma

RolandasRazma Feb 14, 2020

Contributor

else { ?

what about reject? doesn't that need to be called?

}
})
}
Expand Down

0 comments on commit 2da9049

Please sign in to comment.