swift
[concurrency][distributed] Inject invocations of transport.actorReady in init
#39762
Merged

[concurrency][distributed] Inject invocations of transport.actorReady in init #39762

kavon
kavon3 years ago🎉 1

A distributed actor's init and deinit should invoke the ActorTransport's actorReady and resignIdentity functions, respectively, in order to notify the transport of the lifetimes of distributed actor that are bound to the transport. We're currently missing the calls to actorReady (and for failable inits, the resignIdentity), and this PR fixes that.

Resolves: rdar://81783599

kavon kavon force pushed 3 years ago
kavon kavon force pushed 3 years ago
ktoso ktoso added distributed
kavon kavon force pushed 3 years ago
kavon
kavon3 years ago

@swift-ci please smoke test

ktoso
ktoso3 years ago🎉 1

You should be able to go around and remove all the:

  init(transport: ActorTransport) {
    defer { transport.actorReady(self) } // FIXME(distributed): rdar://81783599 this should be injected automatically
  }

now btw.

ktoso
ktoso commented on 2021-10-19
ktoso3 years ago

Looking good so far :)

Conversation is marked as resolved
Show resolved
lib/SILGen/SILGenConstructor.cpp
880 // For distributed actors, their synchronous initializers invoke "actor ready"
881 // at the very end, just before returning on a successful initialization.
882 // There is no need for injecting `resignIdentity` calls in such inits,
883
// because we can never call `actorReady` prematurely.
ktoso3 years ago

Hm... the comment specifically is about "synchronous, non-failable, non-throwing initializers".
A throwing one would have to insert the resign if the class/actor wasn't fully initialized yet and we end up throwing - since the deinit won't be triggered then.

kavon3 years ago

The comment is meant to include all synchronous inits, whether failable or throwing. I'm emitting the actorReady call after the user-defined code has emitted, as one of the very last steps prior to returning within the single control-flow path that represents the non-failing, non-throwing case (since in SILGen we maintain an invariant that there is only one return block). I'll try to clarify this further in the comment.

ktoso3 years ago

I see, thanks for clarifying 👍

Conversation is marked as resolved
Show resolved
lib/SILGen/SILGenConstructor.cpp
885 RegularLocation loc(ctor);
886 loc.markAutoGenerated();
887
888
SILGenSavedInsertionPoint savedIP(*this, ReturnDest.getBlock());
ktoso3 years ago

ah nice <3

Conversation is marked as resolved
Show resolved
lib/SILGen/SILGenDistributed.cpp
ktoso3 years ago

👍

kavon kavon force pushed 3 years ago
kavon kavon force pushed 3 years ago
kavon kavon force pushed 3 years ago
kavon kavon force pushed 3 years ago
kavon
kavon3 years ago (edited 3 years ago)👍 1

At this point, I just need to write SIL tests and update the existing ones (such as removing those defer lines!). What's known to be working:

  • Plain and failable synchronous inits
  • Plain async inits

What's yet to be evaluated or implemented:

  • Throwing synchronous inits
  • Failable and throwing async inits
ktoso
ktoso3 years ago

@swift-ci please smoke test

ktoso
ktoso3 years ago

Failure was weird driver issue, restarting

kavon kavon force pushed 3 years ago
kavon impose `self` restrictions in distributed actor inits
cecdc807
kavon Replace `destoryDistributedActor` builtin with `destroyDefaultActor`
006e2b44
kavon fixed emission of actorReady call in init epilogue
38e6303d
kavon refactor some of SILGen's distributed actor code
8d3c9b0e
kavon invoke resignIdentity in an async failable initializer
b7d5e0af
kavon decouple the emission of actorReady from SILGen
51b76979
kavon inject actorReady calls for async dist actor ctors
e54fa6c6
kavon update dist actor deinit SIL tests
0651acea
kavon merge coverage of multiple transport args into a runtime test
cc1ae9f9
kavon Move the few SIL tests under the Distributed directory
bdd87669
kavon add SIL coverage for current actorReady and resignIdentity calls
efe8f5de
kavon remove redundant defers, now that actorReady is called implicitly
e545557d
kavon capture the known problem with multiple actorReady calls in async inits
f75a6e94
kavon kavon force pushed to f75a6e94 3 years ago
ktoso
ktoso approved these changes on 2021-10-22
lib/SILGen/SILGenDistributed.cpp
254 selfArg = selfArg.borrow(*this, loc);
255 emitTransportInit(*this, ctor, loc, selfArg);
256 emitIdentityInit(*this, ctor, loc, selfArg);
388257
}
ktoso3 years ago🎉 1

thanks for the great cleanups here by the way 👍

lib/SILOptimizer/Transforms/AccessEnforcementReleaseSinking.cpp
182182 case BuiltinValueKind::InitializeDefaultActor:
183183 case BuiltinValueKind::DestroyDefaultActor:
184184 case BuiltinValueKind::InitializeDistributedRemoteActor:
185
case BuiltinValueKind::DestroyDistributedActor:
ktoso3 years ago

yep, good to remove -- thank you

test/Distributed/SIL/distributed_actor_default_init_sil.swift
128
129}
130
131
ktoso3 years ago

Thanks a lot for those tests -- they're annoying to write heh... :)

kavon throwing sync inits are also not fully working
9a76323b
kavon kavon marked this pull request as ready for review 3 years ago
kavon
kavon3 years ago🎉 1

@swift-ci please smoke test and merge

ktoso
ktoso3 years ago

@swift-ci please smoke test and merge

kavon fix whitespace in check line for Linux
191c996d
kavon
kavon3 years ago

@swift-ci please smoke test and merge

kavon
kavon3 years ago

Hm, unrelated failure:

13:28:51 Test Suite 'RepositoryManagerTests' started at 2021-10-22 20:28:38.257
13:28:51 Test Case 'RepositoryManagerTests.testBasics' started at 2021-10-22 20:28:38.257
13:28:51 
/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-main/swiftpm/Tests/SourceControlTests/RepositoryManagerTests.swift:309: error: RepositoryManagerTests.testBasics : Asynchronous wait failed - Exceeded timeout of 1.0 seconds, with unfulfilled expectations: Repository lookup expectation
13:28:51 Test Case 'RepositoryManagerTests.testBasics' failed (2.229 seconds)

@swift-ci please smoke test Linux platform

kavon kavon merged 6061de21 into main 3 years ago
aschwaighofer
aschwaighofer3 years ago

PR testing is blocked. Looks like this could be due to this PR.

I am trying to rectify this here: #39892

Probably, we just need availability restrictions on the attribute in the test for the other platforms too.

Failed Tests (2):
  Swift(iphonesimulator-x86_64) :: Distributed/SIL/distributed_actor_default_deinit_sil.swift
  Swift(iphonesimulator-x86_64) :: Distributed/SIL/distributed_actor_default_init_sil.swift
******************** TEST 'Swift(iphonesimulator-x86_64) :: Distributed/SIL/distributed_actor_default_deinit_sil.swift' FAILED ********************
Script:
--
: 'RUN: at line 1';   /Users/buildnode/jenkins/workspace/oss-swift-pr-test/buildbot_incremental/swift-macosx-x86_64/bin/swift-frontend -target x86_64-apple-ios7.0-simulator  -module-cache-path /Users/buildnode/jenkins/workspace/oss-swift-pr-test/buildbot_incremental/swift-macosx-x86_64/swift-test-results/x86_64-apple-ios7.0-simulator/clang-module-cache -sdk '/Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator15.0.sdk'  -swift-version 4  -define-availability 'SwiftStdlib 5.5:macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0' -typo-correction-limit 10  -module-name default_deinit -primary-file /Users/buildnode/jenkins/workspace/oss-swift-pr-test/swift/test/Distributed/SIL/distributed_actor_default_deinit_sil.swift -emit-sil -enable-experimental-distributed | /Applications/Xcode-beta.app/Contents/Developer/usr/bin/python3 /Users/buildnode/jenkins/workspace/oss-swift-pr-test/swift/utils/PathSanitizingFileCheck --allow-unused-prefixes --sanitize BUILD_DIR=/Users/buildnode/jenkins/workspace/oss-swift-pr-test/buildbot_incremental/swift-macosx-x86_64 --sanitize SOURCE_DIR=/Users/buildnode/jenkins/workspace/oss-swift-pr-test/swift --use-filecheck /Users/buildnode/jenkins/workspace/oss-swift-pr-test/buildbot_incremental/llvm-macosx-x86_64/bin/FileCheck  /Users/buildnode/jenkins/workspace/oss-swift-pr-test/swift/test/Distributed/SIL/distributed_actor_default_deinit_sil.swift --enable-var-scope --dump-input=fail
--
Exit Code: 2

Command Output (stderr):
--
/Users/buildnode/jenkins/workspace/oss-swift-pr-test/swift/test/Distributed/SIL/distributed_actor_default_deinit_sil.swift:10:19: error: concurrency is only available in iOS 13.0.0 or newer
distributed actor MyDistActor {
                 ^
/Users/buildnode/jenkins/workspace/oss-swift-pr-test/swift/test/Distributed/SIL/distributed_actor_default_deinit_sil.swift:10:19: note: add @available attribute to enclosing class
distributed actor MyDistActor {
                 ^
/Users/buildnode/jenkins/workspace/oss-swift-pr-test/swift/test/Distributed/SIL/distributed_actor_default_deinit_sil.swift:10:19: error: protocol 'Hashable' requires 'hash(into:)' to be available in iOS 7.0.0 and newer
distributed actor MyDistActor {

https://ci.swift.org/job/oss-swift-pr-test/18972/consoleText

ktoso
ktoso3 years ago

Thanks @aschwaighofer, hmmm seems the availability annotation was wrong on the added tests;
we generally skip the checks in distributed tests, or when we don't we do @available(SwiftStdlib 5.5, *) which is a lie but that is what we currently "pretend to be" (it's under experimental flags and fine this way).

I'll try to submit a fix.

Login to write a write a comment.

Login via GitHub

Reviewers
Assignees
No one assigned
Labels
Milestone