[Proposal] Update ProgressManager Proposal #1335
Conversation
|
@swift-ci please test |
Proposals/0023-progress-reporter.md
Outdated
| /// | ||
| /// - Parameters: | ||
| /// - property: Type of property. | ||
| /// - values: Sum of values. |
There was a problem hiding this comment.
Is this a copy-pasta leftover?
There was a problem hiding this comment.
Yep, thanks for catching this, will fix this!
| /// | ||
| /// - Parameter property: Type of property. | ||
| /// - Returns: Array of values for property. | ||
| public func values<P: ProgressManager.Property>(of property: P.Type) -> [P.Value] |
There was a problem hiding this comment.
Can we have a sentence or two for this and the below API in the section about properties? I don't think I see the discussion in this proposal
There was a problem hiding this comment.
Yep, I can add this!
Proposals/0023-progress-reporter.md
Outdated
| We considered introducing only two types in this API, `ProgressManager` and `Subprogress`, which would enable developers to create a tree of `ProgressManager` to report progress. However, this has two limitations: | ||
| - It assumes a single-parent, tree-based structure. | ||
| - Developers would have to expose a mutable `ProgressManager` to its observers if they decide to have `ProgressManager` as a property on a class. For example: | ||
| ```swift |
There was a problem hiding this comment.
Nit: I think this indentation would cause this to not be rendered in code block as you intended.
| ```swift | |
| ```swift |
There was a problem hiding this comment.
Okay will fix this!
Proposals/0023-progress-reporter.md
Outdated
|
|
||
| let observedProgress = DownloadManager().progress | ||
| observedProgress.complete(count: 12) // ⚠️: ALLOWED, because `ProgressManager` is mutable!! | ||
| ``` |
Proposals/0023-progress-reporter.md
Outdated
|
|
||
| ### Introduce `totalCount` and `completedCount` properties as `UInt64` | ||
| We considered using `UInt64` as the type for `totalCount` and `completedCount` to support the case where developers use `totalCount` and `completedCount` to track downloads of larger files on 32-bit platforms byte-by-byte. However, developers are not encouraged to update progress byte-by-byte, and should instead set the counts to the granularity at which they want progress to be visibly updated. For instance, instead of updating the download progress of a 10,000 bytes file in a byte-by-byte fashion, developers can instead update the count by 1 for every 1,000 bytes that has been downloaded. In this case, developers set the `totalCount` to 10 instead of 10,000. To account for cases in which developers may want to report the current number of bytes downloaded, we added `totalByteCount` and `completedByteCount` to `ProgressManager.Properties`, which developers can set and display using format style. | ||
|
|
||
| ### Make `totalCount` a settable property on `ProgressManager` | ||
| We previously considered making `totalCount` a settable property on `ProgressManager`, but this would introduce a race condition that is common among cases in which `Sendable` types have settable properties. This is because two threads can try to mutate `totalCount` at the same time, but the `Mutex` guarding `ProgressManager` will not be held across both operations, thus creating a race condition, resulting in the `totalCount` that can either reflects both the mutations, or one of the mutations indeterministically. Therefore, we changed it so that `totalCount` is a read-only property on `ProgressManager`, and is only mutable within the `withProperties` closure to prevent this race condition. |
There was a problem hiding this comment.
I suggest that we remove the mention of mutex here. Using Mutex is an implementation detail, and is not really relevant to the discussion here. It is simply impossible to coordinate this within the class, regardless of the internal synchronization mechanism we use. (I guess the only way to forcefully synchronize this is to make ProgressManager a singleton...) It will always be the callsites' responsibility to make sure they're not mutating a shared variable concurrently.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@swift-ci please test |
ProgressManager Proposal Update