fix(toolkit): keep locks #335
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Associate a read lock with the lifetime of a `CloudAssembly`. The write lock that we take during synthesis gets converted into a read lock after synthesis is done. The presence of long-lasting read locks prevents the cloud assembly direcctory being trampled on by a new synthesis while it is still being read from. In order to keep the return types somewhat compatible (`cxapi.CloudAssembly`) we now return a new type that represents that a `cxapi.CloudAssembly` may or may not have a read lock associated with it. This may not be good enough because it doesn't give a good way of disposing of the lock; in order to be able to use a `using` statement we need an interface that *definitely* has `Symbol.asyncDispose`, not one that *maybe* has it.
mrgrain
reviewed
Apr 4, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 11, 2025
Cloud Assemblies are supposed to be read-locked as they are being used, so that concurrent writers into the same directories do not trample on the `cdk.out` directory as we are deploying it. This is being done with the class `RWLock` which represents a read/write lock. A lock is associated to a directory, which can have either at most one writer or multiple readers. This PR is a synthesis of #335 and #339. Closes aws/aws-cdk#32964, closes #335, closes #339. # Design Because we need to add the concept of a lock to a Cloud Assembly, and we want that resource to be disposable, we introduce the class `IReadableCloudAssembly`. This interface models both a cloud assembly and a dispose function which releases the lock and optionally cleans up the backing directory of the cloud assembly. Cloud Assembly Sources (`ICloudAssemblySource.produce()`) now returns a `IReadableCloudAssembly` with a lock associated. The factory functions start off by acquiring write locks on the backing directory during synthesis, which are converted to read locks after a successful synthesis. * All toolkit functions take an `ICloudAssemblySource`, and dispose of the produced `IReadableCloudAssembly` after use; * Except `synth()` now returns a `CachedCloudAssembly`. That class implements both `IReadableCloudAssembly` (as in, it is a cloud assembly that is locked and ready to be read), as well as `ICloudAssemblySource` so that it can be passed into other toolkit functions. The `CachedCloudAssembly.produce()` returns borrowed versions of `IReadableCloudAssembly` with dispose functions that don't do anything: only the top-level dispose actually cleans anything up. This is so that if the result of `synth()` is passed into (multiple) other toolkit functions, their dispose calls don't accidentally release the lock. This could have been done with reference counting, but is currently being done by giving out a "borrowed" Cloud Assembly which doesn't clean up at all. * The locking itself is managed by `ExecutionEnvironment`; this is now a disposable object which will hold a lock that either gets cleaned up automatically with the object, or get converted to a reader lock when the `ExecutionEnvironment` is explicitly marked as having successfully completed. That same change now allows us to automatically clean up temporary directories if synthesis fails, or when the CloudAssembly in them is disposed of. # Supporting changes Supporting changes in this PR, necessary to make the above work: - `StackAssembly`, which is a helper class to query a Cloud Assembly, used to be an `ICloudAssemblySource` but it no longer is. That functionality isn't used anywhere. - Rename `CachedCloudAssemblySource` to `CachedCloudAssembly` and make `synth()` return this concretely. It makes more sense to think of this class as a Cloud Assembly (that happens to be usable as a source), than the reverse. - Locks can now be released more than once; calls after the first won't do anything. This is to prevent an `_unlock()` followed by a `dispose()` from doing something unexpected, and it also shouldn't fail. - Rename `ILock` => `IReadLock` to contrast it more clearly with an `IWriteLock`. - Remove `IdentityCloudAssemblySource`, since it fills much the same role as `CachedCloudAssembly`. - `ExecutionEnvironment` is now disposable, and its sync constructor has been changed to an async factory function. - A lot of tests that called `cx.produce()` directory now have to use an `await using` statement to make sure the read locks are cleaned up, otherwise they get added to the GitHub repo. # New tests for new contracts - Cloud Assemblies are properly disposed by all toolkit methods that take a CloudAssemblySource - The return value of `toolkit.synth()` can be passed to other toolkit methods, but its contents are only disposed when the object is explicitly disposed, not when it's passed to toolkit methods. - When Cloud Assembly producers fail, they should leave the directory unlocked if given, or no directory at all if they are temporary. # Breaking change BREAKING CHANGE: this change changes the return type of `toolkit.synth()`: it no longer returns an arbitrary Assembly Source (by interface), but a specific Assembly, by class, that can also be used as a source. The return type of `ICloudAssemblySource.produce()` has been changed to `IReadableCloudAssembly`. This will only affect consumers with custom implementations of that interface, the factory function APIs are unchanged. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Associate a read lock with the lifetime of a
CloudAssembly. The write lock that we take during synthesis gets converted into a read lock after synthesis is done.The presence of long-lasting read locks prevents the cloud assembly direcctory being trampled on by a new synthesis while it is still being read from.
In order to keep the return types somewhat compatible (
cxapi.CloudAssembly) we now return a new type that represents that acxapi.CloudAssemblymay or may not have a read lock associated with it.This may not be good enough because it doesn't give a good way of disposing of the lock; in order to be able to use a
usingstatement we need an interface that definitely hasSymbol.asyncDispose, not one that maybe has it.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license