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

String(format:) intermittently returns an empty string when run concurrently by swift-testing #5152

Open
euanh opened this issue Dec 20, 2024 · 1 comment
Assignees

Comments

@euanh
Copy link

euanh commented Dec 20, 2024

The following test, run from the command line in a loop, will eventually fail at random because the String constructor returns an empty string:

import Foundation
import Testing

@Suite struct FormatTest {
    @Test(arguments: [
        (input: 0o000, expected: "000000"),
        (input: 0o555, expected: "000555"),
        (input: 0o750, expected: "000750"),
        (input: 0o777, expected: "000777"),
        (input: 0o1777, expected: "001777"),
    ])
    func testStringFormat(input: Int, expected: String) async throws {
        #expect(String(format: "%06o", input) == expected)
    }
}

On my machine it fails roughly once in every 20 runs, on different test inputs. The error is always that the constructor returns an empty string. The inputs are fixed and always in a range which can be represented in a 6 character octal string.

􀢄  Test testStringFormat(input:expected:) recorded an issue with 2 arguments input → 1023, expected → "001777" at StringTests.swift:13:9: Expectation failed: (String(format: "%06o", input) →  "") == (expected → "001777")

I only noticed this when I started to use swift-testing, which runs test cases in parallel by default. It has never come up in actual use, as far as I'm aware, because I only use this constructor in single-threaded sequential code.

Could there be a shared buffer somewhere in the CFString underpinnings which makes this constructor not threadsafe? I didn't see anything warning about this in the docs: https://developer.apple.com/documentation/swift/string/init(format:_:).


XCode 16.2 / 16C5032a
macOS 15.1.1 (24B91)

% swift -version
swift-driver version: 1.115.1 Apple Swift version 6.0.3 (swiftlang-6.0.3.1.10 clang-1600.0.30.1)
Target: arm64-apple-macosx15.0

@euanh euanh changed the title String(format:) fails intermittently when run concurrently by swift-testing String(format:) intermittently returns an empty string when run concurrently by swift-testing Dec 20, 2024
euanh added a commit to euanh/swift-container-plugin that referenced this issue Dec 20, 2024
String(format:) sometimes returns an empty string even if the number
being formatted can be represented in an octal string of the requested
length.  This may be a thread-safety problem and has only been seen
when running the tests.

    swiftlang/swift-corelibs-foundation#5152

This commit changes octal6/11 to use String(value, radix:) and handle
padding directly.   This has not failed during hundreds of test runs.

Benchmarking the old and new implementations of octal6() shows that
the new version is about twice as fast and makes no allocations, whereas
the old version made 7 allocations.
euanh added a commit to euanh/swift-container-plugin that referenced this issue Dec 20, 2024
String(format:) sometimes returns an empty string even if the number
being formatted can be represented in an octal string of the requested
length.  This may be a thread-safety problem and has only been seen
when running the tests.

    swiftlang/swift-corelibs-foundation#5152

This commit changes octal6/11 to use String(value, radix:) and
handle padding directly.   This has not failed during hundreds of
test runs.

Benchmarking the old and new implementations of octal6() shows that
the new version also is about twice as fast as the old one and makes
no allocations, whereas the old version made 7 allocations.
@jmschonfeld jmschonfeld self-assigned this Jan 3, 2025
euanh added a commit to euanh/swift-container-plugin that referenced this issue Jan 6, 2025
String(format:) sometimes returns an empty string even if the number
being formatted can be represented in an octal string of the requested
length.  This may be a thread-safety problem and has only been seen
when running the tests.

    swiftlang/swift-corelibs-foundation#5152

This commit changes octal6/11 to use String(value, radix:) and
handle padding directly.   This has not failed during hundreds of
test runs.

Benchmarking the old and new implementations of octal6() shows that
the new version also is about twice as fast as the old one and makes
no allocations, whereas the old version made 7 allocations.
euanh added a commit to euanh/swift-container-plugin that referenced this issue Jan 6, 2025
String(format:) sometimes returns an empty string even if the number
being formatted can be represented in an octal string of the requested
length.  This may be a thread-safety problem and has only been seen
when running the tests.

    swiftlang/swift-corelibs-foundation#5152

This commit changes octal6/11 to use String(value, radix:) and
handle padding directly.   This has not failed during hundreds of
test runs.

Benchmarking the old and new implementations of octal6() shows that
the new version also is about twice as fast as the old one and makes
no allocations, whereas the old version made 7 allocations.
euanh added a commit to apple/swift-container-plugin that referenced this issue Jan 6, 2025
Motivation
----------

The `tar` writer is tested by the end to end tests, but unit tests are
more helpful for refactoring and extending it.

Modifications
-------------
 
* Extract `tar` into its own module
* Extract tar header construction into a separate function
* Add initial unit tests for the helper methods used to build `tar`
headers
* Stop using `Swift(format:)` to convert integers into octal strings
because of
swiftlang/swift-corelibs-foundation#5152

Result
------

The basic helper functions underpinning the tar writer will have unit
tests, making it easier to test, refactor and extend the tar writer in
future.

Test Plan
---------

This pull request adds new tests. All existing tests continue to pass.
@jmschonfeld
Copy link
Contributor

@euanh thanks for reporting this! I've found the root cause of the issue and addressed it in this copy of CoreFoundation that ships with Swift on Linux/Windows at #5155. We'll make sure that the copy of CoreFoundation that ships with Darwin platforms is also updated with the fix so that platforms like macOS are corrected as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants