Skip to content

Fix occasional flakiness in preview action cancellation test #1070

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ struct SymbolGraphLoader {

let loadGraphAtURL: (URL) -> Void = { [dataLoader, bundle] symbolGraphURL in
// Bail out in case a symbol graph has already errored
guard loadError == nil else { return }
guard loadingLock.sync({ loadError == nil }) else { return }

do {
// Load and decode a single symbol graph file
Expand Down
7 changes: 5 additions & 2 deletions Sources/SwiftDocC/Utility/LogHandle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ public enum LogHandle: TextOutputStream {

/// A by-reference string storage.
public class LogStorage {
var text = ""
var _text = Synchronized("")
var text: String {
_text.sync { $0 }
}
}

/// Writes the given string to the log handle.
Expand All @@ -51,7 +54,7 @@ public enum LogHandle: TextOutputStream {
case .file(let fileHandle):
fileHandle.write(Data(string.utf8))
case .memory(let storage):
storage.text.append(string)
storage._text.sync { $0.append(string) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to synchronize fileHandle in the same way? Or does macOS/Linux guarantee that writing to a file is thread safe - i.e. synchronized at a lower level?

Same question for stdout and stderr...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't ever encountered any issues with writing to a file and I've never seen a pattern of synchronizing around fileHandle.write or print but I can't say for sure.

}
}
}
Expand Down
8 changes: 4 additions & 4 deletions Sources/SwiftDocC/Utility/Synchronization.swift
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,16 @@ public class Synchronized<Value> {
/// lock.sync { myVar = 1 }
/// let result = lock.sync { return index["key"] }
/// ```
typealias Lock = Synchronized<Void>
package typealias Lock = Synchronized<Void>

public extension Lock {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to mark this extension as non-public? Could some client package be using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's safe because it's extending an internal type so its API was never actually available to the public.

Because Lock was defined as typealias Lock = Synchronized<Void> (no access modifier) it was never publicly available so a public extension to Lock could never exceed the Lock types internal access. I feel that there should have been a warning about this but there wasn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only found this because I tried to use this API from SwiftDocCUtilities and got a build error that it could't be accessed because it was internal.

extension Lock {
/// Creates a new lock.
convenience init() {
package convenience init() {
self.init(())
}

@discardableResult
func sync<Result>(_ block: () throws -> Result) rethrows -> Result {
package func sync<Result>(_ block: () throws -> Result) rethrows -> Result {
#if os(macOS) || os(iOS)
os_unfair_lock_lock(lock)
defer { os_unfair_lock_unlock(lock) }
Expand Down
57 changes: 27 additions & 30 deletions Sources/SwiftDocCUtilities/Action/Actions/PreviewAction.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ public final class PreviewAction: AsyncAction {
static var allowConcurrentPreviews = false

private let printHTMLTemplatePath: Bool

var logHandle = LogHandle.standardOutput

let port: Int

Expand Down Expand Up @@ -89,17 +87,17 @@ public final class PreviewAction: AsyncAction {
///
/// - Parameter logHandle: The file handle that the convert and preview actions will print debug messages to.
public func perform(logHandle: inout LogHandle) async throws -> ActionResult {
self.logHandle = logHandle
self.logHandle.sync { $0 = logHandle }

if let rootURL = convertAction.rootURL {
print("Input: \(rootURL.path)", to: &self.logHandle)
print("Input: \(rootURL.path)")
}
// TODO: This never did output human readable string; rdar://74324255
// print("Input: \(convertAction.documentationCoverageOptions)", to: &self.logHandle)

// In case a developer is using a custom template log its path.
if printHTMLTemplatePath, let htmlTemplateDirectory = convertAction.htmlTemplateDirectory {
print("Template: \(htmlTemplateDirectory.path)", to: &self.logHandle)
print("Template: \(htmlTemplateDirectory.path)")
}

let previewResult = try await preview()
Expand All @@ -124,15 +122,16 @@ public final class PreviewAction: AsyncAction {
let previewResult: ActionResult
// Preview the output and monitor the source bundle for changes.
do {
print(String(repeating: "=", count: 40), to: &logHandle)
print(String(repeating: "=", count: 40))
if let previewURL = URL(string: "http://localhost:\(port)") {
print("Starting Local Preview Server", to: &logHandle)
print("Starting Local Preview Server")
printPreviewAddresses(base: previewURL)
print(String(repeating: "=", count: 40), to: &logHandle)
print(String(repeating: "=", count: 40))
}

let to: PreviewServer.Bind = bindServerToSocketPath.map { .socket(path: $0) } ?? .localhost(port: port)
servers[serverIdentifier] = try PreviewServer(contentURL: convertAction.targetDirectory, bindTo: to, logHandle: &logHandle)
var logHandleCopy = logHandle.sync { $0 }
servers[serverIdentifier] = try PreviewServer(contentURL: convertAction.targetDirectory, bindTo: to, logHandle: &logHandleCopy)

// When the user stops docc - stop the preview server first before exiting.
trapSignals()
Expand All @@ -159,7 +158,8 @@ public final class PreviewAction: AsyncAction {

func convert() async throws -> ActionResult {
convertAction = try createConvertAction()
let (result, context) = try await convertAction.perform(logHandle: &logHandle)
var logHandleCopy = logHandle.sync { $0 }
let (result, context) = try await convertAction.perform(logHandle: &logHandleCopy)

previewPaths = try context.previewPaths()
return result
Expand All @@ -168,15 +168,23 @@ public final class PreviewAction: AsyncAction {
private func printPreviewAddresses(base: URL) {
// If the preview paths are empty, just print the base.
let firstPath = previewPaths.first ?? ""
print("\t Address: \(base.appendingPathComponent(firstPath).absoluteString)", to: &logHandle)
print("\t Address: \(base.appendingPathComponent(firstPath).absoluteString)")

let spacing = String(repeating: " ", count: "Address:".count)
for previewPath in previewPaths.dropFirst() {
print("\t \(spacing) \(base.appendingPathComponent(previewPath).absoluteString)", to: &logHandle)
print("\t \(spacing) \(base.appendingPathComponent(previewPath).absoluteString)")
}
}

var monitoredConvertTask: Task<Void, Never>?
private var logHandle: Synchronized<LogHandle> = .init(.none)

fileprivate func print(_ string: String, terminator: String = "\n") {
logHandle.sync { logHandle in
Swift.print(string, terminator: terminator, to: &logHandle)
}
}

fileprivate var monitoredConvertTask: Task<Void, Never>?
}

// Monitoring a source folder: Asynchronous output reading and file system events are supported only on macOS.
Expand All @@ -192,38 +200,27 @@ extension PreviewAction {
}

monitor = try DirectoryMonitor(root: rootURL) { _, _ in
print("Source bundle was modified, converting... ", terminator: "", to: &self.logHandle)
self.print("Source bundle was modified, converting... ", terminator: "")
self.monitoredConvertTask?.cancel()
self.monitoredConvertTask = Task {
defer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we need this defer block any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell it wasn't ever needed. The DirectoryMonitor already does a smarter automatic reloading when files are added to or removed from a monitored directory.

Removing the restart here means that we don't:

  • Restart (stop monitoring all files and directories and start monitoring them again) twice when a file is added removed (once here and once in the directory monitor itself).
  • Restart (stop monitoring all files and directories and start monitoring them again) when an already monitored file's content changed but no files were added or removed.

// Reload the directory contents and start to monitor for changes.
do {
try monitor.restart()
} catch {
// The file watching system API has thrown, stop watching.
print("Watching for changes has failed. To continue preview with watching restart docc.", to: &self.logHandle)
print(error.localizedDescription, to: &self.logHandle)
}
}

do {
let result = try await self.convert()
if result.didEncounterError {
throw ErrorsEncountered()
}
print("Done.", to: &self.logHandle)
self.print("Done.")
} catch DocumentationContext.ContextError.registrationDisabled {
// The context cancelled loading the bundles and threw to yield execution early.
print("\nConversion cancelled...", to: &self.logHandle)
self.print("\nConversion cancelled...")
} catch is CancellationError {
print("\nConversion cancelled...", to: &self.logHandle)
self.print("\nConversion cancelled...")
} catch {
print("\n\(error.localizedDescription)\nCompilation failed", to: &self.logHandle)
self.print("\n\(error.localizedDescription)\nCompilation failed")
}
}
}
try monitor.start()
print("Monitoring \(rootURL.path) for changes...", to: &self.logHandle)
self.print("Monitoring \(rootURL.path) for changes...")
}
}
#endif // !os(Linux) && !os(Android)
Expand Down
34 changes: 20 additions & 14 deletions Sources/SwiftDocCUtilities/Utility/DirectoryMonitor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -207,18 +207,22 @@ class DirectoryMonitor {
)
}

private var startStopLock = Lock()

/// Start monitoring the root directory and its contents.
func start() throws {
let watchedFiles = try watchedURLChecksum()
lastChecksum = watchedFiles.checksum

do {
watchedDirectories = try allDirectories(in: root)
.map { return try watchedDirectory(at: $0.directory, files: $0.files, on: monitorQueue) }
} catch Error.openFileHandleFailed(_, let errorCode) where errorCode == Error.tooManyOpenFilesErrorCode {
// In case we've reached the file descriptor limit throw a detailed user error
// with recovery instructions.
throw Error.reachedOpenFileLimit(watchedFiles.count)
try startStopLock.sync {
let watchedFiles = try watchedURLChecksum()
lastChecksum = watchedFiles.checksum

do {
watchedDirectories = try allDirectories(in: root)
.map { return try watchedDirectory(at: $0.directory, files: $0.files, on: monitorQueue) }
} catch Error.openFileHandleFailed(_, let errorCode) where errorCode == Error.tooManyOpenFilesErrorCode {
// In case we've reached the file descriptor limit throw a detailed user error
// with recovery instructions.
throw Error.reachedOpenFileLimit(watchedFiles.count)
}
}
}

Expand All @@ -229,12 +233,14 @@ class DirectoryMonitor {

/// Stop monitoring.
func stop() {
for directory in watchedDirectories {
for source in directory.sources {
source.cancel()
startStopLock.sync {
for directory in watchedDirectories {
for source in directory.sources {
source.cancel()
}
}
watchedDirectories = []
}
watchedDirectories = []
}

deinit {
Expand Down
Loading