Fix memory leaks in ThreadBarriers.swift #12212

chrisamanse · 2017-10-02T06:05:22Z

Fix memory leaks in PthreadBarriers.swift as indicated by FIXMEs.

jrose-apple · 2017-10-02T20:31:45Z

stdlib/private/SwiftPrivatePthreadExtras/PthreadBarriers.swift

-    // FIXME: leaking memory, leaking a mutex.
+  guard pthread_cond_init(barrier.pointee.cond!, nil) == 0 else {
+    barrier.pointee.cond!.deinitialize()
+    barrier.pointee.cond!.deallocate(capacity: 1)


The mutex would also have to be cleaned up here. Maybe the whole thing belongs in a defer that just checks that barrier.pointee.count is non-zero?

Or this can be merged into one guard statement:

guard pthread_mutex_init(barrier.pointee.mutex!, nil) == 0 && pthread_cond_init(barrier.pointee.cond!, nil) == 0 else { barrier.pointee.mutex!.deinitialize() barrier.pointee.mutex!.deallocate(capacity: 1) barrier.pointee.cond!.deinitialize() barrier.pointee.cond!.deallocate(capacity: 1) return -1 }

If one of them fails, both mutex and cond will be deallocated. What do you think?

This still doesn't call pthread_mutex_destroy when pthread_mutex_init succeeds but pthread_cond_init fails.

Understood. Updating code to destroy mutex when pthread_cond_init fails.

CodaFi · 2019-11-14T19:17:08Z

@airspeedswift Is this change still good to take (with some rebasing?)

mikeash · 2019-11-19T15:25:41Z

stdlib/private/SwiftPrivatePthreadExtras/PthreadBarriers.swift

-  if pthread_mutex_destroy(barrier.pointee.mutex!) != 0 {
-    // FIXME: leaking memory.
+  guard pthread_cond_destroy(barrier.pointee.cond!) == 0 &&
+    pthread_mutex_destroy(barrier.pointee.mutex!) == 0 else {


Failures when releasing resources kind of weird me out. This would leak or leave the barrier in an invalid state if pthread_cond_destroy fails.

According to the man page, this can only fail due to API abuse (either passing an invalid parameter, or trying to destroy it while it's in use by another thread). The callers of this function either fatalError or signal a test failure. I'm wondering if we should just not even try to pretend that this failure can be handled gracefully, and just abort here instead.

Good point. The caller of this function in SwiftPrivatePthreadExtras.swift does fatalError when this returns -1. Should this function simply just return Void and fatalError in here?

I think so. If somebody wants to try to handle these errors gracefully one day, they can figure out how. No need to do that work now, only to immediately fatalError anyway.

mikeash

👍

CodaFi · 2019-12-09T17:02:58Z

@swift-ci please test

swift-ci · 2019-12-09T17:34:08Z

Build failed
Swift Test OS X Platform
Git Sha - 315f510

swift-ci · 2019-12-09T17:48:48Z

Build failed
Swift Test Linux Platform
Git Sha - 315f510

chrisamanse · 2019-12-09T22:35:21Z

/home/buildnode/jenkins/workspace/swift-PR-Linux@2/branch-master/swift/stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift:84:90: error: use of undeclared type '_stdlib_pthread_barrier_t'

My bad, that has been renamed to _stdlib_thread_barrier_t

CodaFi · 2019-12-09T23:46:14Z

@swift-ci please test

swift-ci · 2019-12-09T23:58:10Z

Build failed
Swift Test Linux Platform
Git Sha - 9e80f27

theblixguy · 2019-12-10T00:11:21Z

Seems like you need to fix the call to pthread_mutex_init & pthread_cond_init:

/home/buildnode/jenkins/workspace/swift-PR-Linux@2/branch-master/swift/stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift:85:50: error: missing argument for parameter #2 in call
17:55:49 guard pthread_mutex_init(barrier.pointee.mutex!) == 0 else {
17:55:49 ^
17:55:49 , <#UnsafePointer<pthread_mutexattr_t>?#>
17:55:49 SwiftGlibc.pthread_mutex_init:1:13: note: 'pthread_mutex_init' declared here
17:55:49 public func pthread_mutex_init(_ __mutex: UnsafeMutablePointer<pthread_mutex_t>, _ _mutexattr: UnsafePointer<pthread_mutexattr_t>!) -> Int32
17:55:49 ^
17:55:49 /home/buildnode/jenkins/workspace/swift-PR-Linux@2/branch-master/swift/stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift:88:48: error: missing argument for parameter #2 in call
17:55:49 guard pthread_cond_init(barrier.pointee.cond!) == 0 else {
17:55:49 ^
17:55:49 , <#UnsafePointer<pthread_condattr_t>?#>
17:55:49 SwiftGlibc.pthread_cond_init:1:13: note: 'pthread_cond_init' declared here
17:55:49 public func pthread_cond_init( __cond: UnsafeMutablePointer<pthread_cond_t>, _ __cond_attr: UnsafePointer<pthread_condattr_t>!) -> Int32
17:55:49

swift-ci · 2019-12-10T00:48:47Z

Build failed
Swift Test OS X Platform
Git Sha - 9e80f27

theblixguy · 2019-12-10T23:26:00Z

@swift-ci please test

swift-ci · 2019-12-10T23:28:28Z

Build failed
Swift Test Linux Platform
Git Sha - 9e80f27

swift-ci · 2019-12-10T23:38:04Z

Build failed
Swift Test OS X Platform
Git Sha - 9e80f27

CodaFi · 2019-12-11T08:00:03Z

stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift

+    barrier.pointee.mutex!.deinitialize(count: 1)
+    barrier.pointee.mutex!.deallocate()
+    barrier.pointee.cond!.deinitialize(count: 1)
+    barrier.pointee.cond!.deallocate()


This has to exit the enclosing scope.

Thanks! Rebasing isn't as straightforward as I thought, I'll try building locally this time

chrisamanse · 2019-12-11T21:20:49Z

Ran ./utils/build-script --release-debuginfo locally. Seems to work, the command ended with:

[1775/1775][100%][1095.175s] Running utility command for swift-benchmark-macosx-x86_64

CodaFi · 2020-01-28T21:26:17Z

@swift-ci test

CodaFi · 2020-01-29T16:21:46Z

Thanks for sticking with us.

⛵️

drodriguez · 2020-01-29T21:56:05Z

Just a note, it broke Windows: https://ci-external.swift.org/job/oss-swift-windows-x86_64/2645/ (/cc @compnerd)

chrisamanse · 2020-01-29T22:19:15Z

Just a note, it broke Windows: https://ci-external.swift.org/job/oss-swift-windows-x86_64/2645/ (/cc @compnerd)

Thanks for catching this one! I thought GitHub did a check for all platforms. I created this PR to fix it #29536

jrose-apple reviewed Oct 2, 2017

View reviewed changes

jrose-apple requested a review from airspeedswift October 2, 2017 20:32

CodaFi assigned airspeedswift Nov 14, 2019

CodaFi requested a review from mikeash November 19, 2019 00:53

mikeash reviewed Nov 19, 2019

View reviewed changes

Chris Amanse added 2 commits December 4, 2019 14:34

Fix memory leaks in ThreadBarriers.swift

38377df

Fatal error on pthread cond/mutex destroy failure

315f510

chrisamanse force-pushed the fix-pthread-leaks branch from 22a91d4 to 315f510 Compare December 4, 2019 22:44

chrisamanse requested a review from mikeash December 7, 2019 01:21

mikeash approved these changes Dec 9, 2019

View reviewed changes

Rename pthread to thread

9e80f27

Fix pthread init function calls

f0e31ee

chrisamanse changed the title ~~Fix memory leaks in PthreadBarriers.swift~~ Fix memory leaks in ThreadBarriers.swift Dec 10, 2019

CodaFi reviewed Dec 11, 2019

View reviewed changes

Fix guard statement

c1e6cf8

CodaFi merged commit ffa6bf4 into swiftlang:master Jan 29, 2020

chrisamanse mentioned this pull request Jan 29, 2020

Private method should be for non-Windows only #29536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leaks in ThreadBarriers.swift #12212

Fix memory leaks in ThreadBarriers.swift #12212

chrisamanse commented Oct 2, 2017

jrose-apple Oct 2, 2017

chrisamanse Oct 3, 2017

gparker42 Oct 3, 2017 •

edited

Loading

chrisamanse Oct 3, 2017

CodaFi commented Nov 14, 2019

mikeash Nov 19, 2019

chrisamanse Nov 20, 2019

mikeash Nov 20, 2019

mikeash left a comment

CodaFi commented Dec 9, 2019

swift-ci commented Dec 9, 2019

swift-ci commented Dec 9, 2019

chrisamanse commented Dec 9, 2019

CodaFi commented Dec 9, 2019

swift-ci commented Dec 9, 2019

theblixguy commented Dec 10, 2019 •

edited

Loading

swift-ci commented Dec 10, 2019

theblixguy commented Dec 10, 2019

swift-ci commented Dec 10, 2019

swift-ci commented Dec 10, 2019

CodaFi Dec 11, 2019

chrisamanse Dec 11, 2019

chrisamanse commented Dec 11, 2019

CodaFi commented Jan 28, 2020

CodaFi commented Jan 29, 2020

drodriguez commented Jan 29, 2020

chrisamanse commented Jan 29, 2020

Fix memory leaks in ThreadBarriers.swift #12212

Fix memory leaks in ThreadBarriers.swift #12212

Conversation

chrisamanse commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gparker42 Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeash left a comment

Choose a reason for hiding this comment

CodaFi commented Dec 9, 2019

swift-ci commented Dec 9, 2019

swift-ci commented Dec 9, 2019

chrisamanse commented Dec 9, 2019

CodaFi commented Dec 9, 2019

swift-ci commented Dec 9, 2019

theblixguy commented Dec 10, 2019 • edited Loading

swift-ci commented Dec 10, 2019

theblixguy commented Dec 10, 2019

swift-ci commented Dec 10, 2019

swift-ci commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisamanse commented Dec 11, 2019

CodaFi commented Jan 28, 2020

CodaFi commented Jan 29, 2020

drodriguez commented Jan 29, 2020

chrisamanse commented Jan 29, 2020

gparker42 Oct 3, 2017 •

edited

Loading

theblixguy commented Dec 10, 2019 •

edited

Loading