Fix memory leak in Lock

[orobio]

The platform lock is never deallocated, which results in a leak. Since the platform lock is owned by Lock, I thought it would be fine to make deallocation part of deinitialize() and not have a separate deallocate() function. Especially since initialization of the lock is also mixed into allocate() already.

[xtremekforever]

This appears to fix the memleak we saw with cancelOnGracefulShutdown() in the swift-service-lifecycle library. I opened an issue* for this here detailing the behavior of this leak:

https://github.com/swift-server/swift-service-lifecycle/issues/188

[phausler]

that is a bit odd since the deinitialize should be triggered by the ManagedBuffer itself

[orobio]

I see what you mean and I must say that I completely overlooked the ManagedBuffer part. I came to Lock from MergeStorage, where the lock is manually allocated and deinitialized. I guess it would be more correct to use ManagedCriticalState here, or was there a reason that it was implemented differently for MergeStorage?

[phausler]

Yea, we should not be using Lock outside of the ManagedCriticalState container; id have to audit the MergeStorage to ensure that is not required there for some reason (like the lock being donated etc)

[FranzBusch]

Yea, we should not be using Lock outside of the ManagedCriticalState container; id have to audit the MergeStorage to ensure that is not required there for some reason (like the lock being donated etc)

There is a reason for using the Lock manually there since it allows us to lock while taking a continuation (a totally safe thing to do). This can't be done with scoped based locking so we have to do manual locking. Doing this allows us to avoid a race conditions which would otherwise we would need to handle and take the lock twice in case we have to create a continuation.

[FranzBusch]

IMO we should merge this since it is the right thing to do.

[FranzBusch]

@swift-ci please test

[phausler]

This should not have gotten merged, it is now double freeing the lock pointer inside the ManagedCriticalState.

[xtremekforever]

Oh dear... 😮 what's the correct solution then? Something needs to free the lock at the right time?

[orobio]

There are 2 deinitialize functions. This one is an instance method that takes no arguments. It is only used from MergeStorage and here is where the the deallocation was added. The other deinitialize function is a static method that takes a PlatformLock as an argument. The static method is used by the instance method to do the actual deinitialization, and it is the one used by ManagedCriticalState. It does not perform deallocation.

A less confusing solution could perhaps be created, but the current code should be correct.

[FranzBusch]

This looks correct to me. The code in question here does indeed not change anything for ManagedCriticalState unless I am missing something. @phausler Do you mind double checking?

[phausler]

hmm yea, after re-looking at it I was looking at the wrong deinit methods - so false alarm on the merge status; perhaps it might be better for us to in the near future to migrate to the Synchronization module instead of rolling our own ManagedCriticalState in this package (which would eliminate this issue completely)

[FranzBusch]

I don't think we can migrate to that module without doing a new SemVer major since the types in that module require a new enough Swift compiler and also have a pretty high availability which would need to bubble up to our types.