Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote target (for distributed actor) arguments are not properly deinitialized #65853

Closed
ser-0xff opened this issue May 11, 2023 · 9 comments · Fixed by #65874 or #65952
Closed

Remote target (for distributed actor) arguments are not properly deinitialized #65853

ser-0xff opened this issue May 11, 2023 · 9 comments · Fixed by #65874 or #65952
Assignees
Labels
actor Feature → concurrency: `actor` declarations bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler itself concurrency Feature: umbrella label for concurrency language features distributed Feature → concurrency: distributed actor memory leak bug: Memory leak swift 5.9

Comments

@ser-0xff
Copy link

ser-0xff commented May 11, 2023

Remote target arguments reconstructed on the remote side of the distributed actor are not properly deinitialised.
If the remote target argument is a struct with some class instance references then those class instances are never released, which cause a memory leak.

We implemented minimised test
It perform a remote call on the distributed actor with a struct argument having a reference to the ClassValue instance.
Can find the ClassValue instance initialized twice (first time on the caller side, second time on the remote side), but reinitialised only once (on the caller side):

git clone https://github.com/ordo-one/external-reproducers.git
Cloning into 'external-reproducers'...
remote: Enumerating objects: 135, done.
remote: Counting objects: 100% (135/135), done.
remote: Compressing objects: 100% (88/88), done.
remote: Total 135 (delta 49), reused 91 (delta 23), pack-reused 0
Receiving objects: 100% (135/135), 27.13 KiB | 524.00 KiB/s, done.
Resolving deltas: 100% (49/49), done.
aaa % cd external-reproducers/swift/distributed-system-leak 
distributed-system-leak % swift test
Building for debugging...
[5/5] Linking DistributedSystemPackageTests
Build complete! (7.65s)
Test Suite 'All tests' started at 2023-05-11 12:45:03.890
Test Suite 'DistributedSystemPackageTests.xctest' started at 2023-05-11 12:45:03.891
Test Suite 'DistributedSystemTests' started at 2023-05-11 12:45:03.891
Test Case '-[DistributedSystemTests.DistributedSystemTests test1]' started.
Test Case '-[DistributedSystemTests.DistributedSystemTests test1]' passed (0.002 seconds).
Test Suite 'DistributedSystemTests' passed at 2023-05-11 12:45:03.893.
	 Executed 1 test, with 0 failures (0 unexpected) in 0.002 (0.002) seconds
Test Suite 'DistributedSystemPackageTests.xctest' passed at 2023-05-11 12:45:03.893.
	 Executed 1 test, with 0 failures (0 unexpected) in 0.002 (0.002) seconds
Test Suite 'All tests' passed at 2023-05-11 12:45:03.893.
	 Executed 1 test, with 0 failures (0 unexpected) in 0.002 (0.003) seconds
assign<TestActor>: 1
actorReady<TestActor>: 1
resolve<TestActor>
init: 0x0000600002a301d0
recordArgument
remoteCallVoid
init: 0x0000600002a301f0
value=100
deinit: 0x0000600002a301d0
distributed-system-leak % 

We would expect the second ClassValue instance constructed on the remote side would be deinitialised, thus the output would also had a line

deinit: 0x0000600002a301f0

Environment

  • Swift compiler version info
distributed-system-leak % swift --version
swift-driver version: 1.62.15 Apple Swift version 5.7.2 (swiftlang-5.7.2.135.5 clang-1400.0.29.51)
Target: arm64-apple-macosx13.0
  • Xcode version info
  • Deployment target:
distributed-system-leak % uname -a
Darwin mbpro.local 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:38:37 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6000 arm64
@ser-0xff ser-0xff added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. triage needed This issue needs more specific labels labels May 11, 2023
@ktoso ktoso added the distributed Feature → concurrency: distributed actor label May 11, 2023
@ktoso ktoso self-assigned this May 11, 2023
@ktoso
Copy link
Contributor

ktoso commented May 11, 2023

Thanks let me try to reproduce this

@ktoso ktoso added memory leak bug: Memory leak and removed triage needed This issue needs more specific labels labels May 11, 2023
@ktoso
Copy link
Contributor

ktoso commented May 11, 2023

Thanks, I was able to reproduce this -- we didn't really test with messages being classes as that's a bit unusual but of course it must be supported correctly. I'm investigating this more now.

@ktoso
Copy link
Contributor

ktoso commented May 11, 2023

rdar://109207043

@ser-0xff
Copy link
Author

Thank you for the update.

@AnthonyLatsis AnthonyLatsis added concurrency Feature: umbrella label for concurrency language features compiler The Swift compiler itself swift 5.9 actor Feature → concurrency: `actor` declarations labels May 11, 2023
xedin added a commit to xedin/swift that referenced this issue May 11, 2023
Arguments that have been loaded due to underlying method conversion
have to be destroyed after the call otherwise they are going to leak.

Resolves: swiftlang#65853
Resolves: rdar://109207043
@ktoso
Copy link
Contributor

ktoso commented May 12, 2023

We reproduced and have a fix incoming, thank you very much for noticing this.

xedin added a commit to xedin/swift that referenced this issue May 15, 2023
Arguments that have been loaded due to underlying method conversion
have to be destroyed after the call otherwise they are going to leak.

Resolves: swiftlang#65853
Resolves: rdar://109207043
@hassila
Copy link

hassila commented May 16, 2023

Super, thanks! Just for our planning (whether we need to workaround this or not), is this going into 5.9 or is it later?

ktoso pushed a commit to ktoso/swift that referenced this issue May 16, 2023
Arguments that have been loaded due to underlying method conversion
have to be destroyed after the call otherwise they are going to leak.

Resolves: swiftlang#65853
Resolves: rdar://109207043
@ktoso
Copy link
Contributor

ktoso commented May 16, 2023

There's no hard promises with these things but yes we've cherry picked it over to 5.9 as well, it should be able to land there AFAICS.

@ktoso ktoso reopened this May 19, 2023
@ktoso
Copy link
Contributor

ktoso commented May 19, 2023

ktoso pushed a commit to ktoso/swift that referenced this issue May 20, 2023
Arguments that have been loaded due to underlying method conversion
have to be destroyed after the call otherwise they are going to leak.

Resolves: swiftlang#65853
Resolves: rdar://109207043
meg-gupta pushed a commit to meg-gupta/swift that referenced this issue May 22, 2023
Arguments that have been loaded due to underlying method conversion
have to be destroyed after the call otherwise they are going to leak.

Resolves: swiftlang#65853
Resolves: rdar://109207043
NuriAmari pushed a commit to NuriAmari/swift that referenced this issue May 28, 2023
Arguments that have been loaded due to underlying method conversion
have to be destroyed after the call otherwise they are going to leak.

Resolves: swiftlang#65853
Resolves: rdar://109207043
@ser-0xff
Copy link
Author

ser-0xff commented Jun 2, 2023

Hi!
We run our minimised test with the new swift toolchain and see there is no leak now.
Thank you for fixing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actor Feature → concurrency: `actor` declarations bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler itself concurrency Feature: umbrella label for concurrency language features distributed Feature → concurrency: distributed actor memory leak bug: Memory leak swift 5.9
Projects
None yet
4 participants