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

[SR-1042] Make "lazy var" threadsafe #43654

Open
drewcrawford opened this issue Mar 23, 2016 · 9 comments
Open

[SR-1042] Make "lazy var" threadsafe #43654

drewcrawford opened this issue Mar 23, 2016 · 9 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself feature A feature request or implementation runtime The Swift Runtime

Comments

@drewcrawford
Copy link
Contributor

Previous ID SR-1042
Radar None
Original Reporter @drewcrawford
Type Bug
Environment

swift-DEVELOPMENT-SNAPSHOT-2016-03-16-a

Additional Detail from JIRA
Votes 8
Component/s Compiler
Labels Bug, LanguageFeatureRequest, Runtime
Assignee None
Priority Medium

md5: 3e0b2bd438a588ec34ca8ff1b3ddeff2

is duplicated by:

  • SR-7712 Sync dispatch queue block crash

Issue Description:

In the Swift book it says

If a property marked with the lazy modifier is accessed by multiple threads simultaneously and the property has not yet been initialized, there is no guarantee that the property will be initialized only once.

Meanwhile, libdispatch maintainers have decided to make dispatch_once unavailable to Swift and advising to use lazy modifier as a replacement.

As a result, there is currently no safe way to achieve "run exactly once" behavior in Swift.

We need some way to do this. I propose the easiest path is to make "lazy var" threadsafe, but if not, there should be some API (stdlib, libdispatch, I don't care) to achieve the "run exactly once" behavior.

I am tentatively marking this "compiler", but feel free to move it around.

@drewcrawford
Copy link
Contributor Author

cc: dgrove-oss (JIRA User) @parkera

@belkadan
Copy link
Contributor

static let is a thread-safe single-initialization mechanism. I think it's not possible to make a thread-safe single-initialization mechanism for a property without external synchronization, but I suppose there could be a side table like the old @swift-cihronized(self).

(IIRC dispatch_once was never safe for instance properties in Objective-C either. See this StackOverflow answer by @gparker42.)

@belkadan
Copy link
Contributor

I think you'll also get pushback from people who want performance, but maybe we'll have lazy and lazy(singlethreaded) or something. Needs design.

@jckarter
Copy link
Member

This seems like something property behaviors could eventually address as a library feature too.

@gparker42
Copy link
Mannequin

gparker42 mannequin commented Mar 23, 2016

A thread-safe lazy property would require a memory barrier on every read on some architectures. (`dispatch_once` avoids that barrier, but it is unsafe for anything other than global variables.)

A thread-safe lazy property that is too large or does not have some unused sentinel value available would also need additional storage for synchronization (stored next to property value, or in the object header, or in a side table like `@swift-cihronized`).

Both of these are expensive and may not be suitable for every lazy property.

@ddunbar
Copy link
Member

ddunbar commented May 23, 2016

+1 to this idea. Not only is there no guarantee that the value will be initialized only once, concurrent access to a lazy variable may cause crashes in otherwise safe code.

My current view is that using "lazy" is almost an anti-pattern for writing complicated concurrent Swift code because it is so easy for code to "look" safe.

The other reason it would be nice to address this in the compiler is that it is hard to come up with a convenient library method for this behavior (I at least haven't been able to get one). What I currently use for this is something like:
apple/swift-package-manager@a086f6c
but that implementation is both cumbersome to use and rather inefficient.

@ddunbar
Copy link
Member

ddunbar commented Mar 9, 2017

This continues to be a regular source of crashes when people forget and use `lazy` in otherwise safe looking code.

This does not seem like the right default for a "safe by default" language.

@belkadan
Copy link
Contributor

belkadan commented Mar 9, 2017

I'm not sure if this needs the full Swift Evolution Process, but discussion should certainly happen on swift-evolution, not here.

@swift-ci
Copy link
Collaborator

Comment by Francisco (JIRA)

Hello! I will share my experience with a nasty bug that happened today to a bunch of users of our app (Mimo).
Basically, I had something in pseudocode like this:

class MyViewController: UIViewController {    

    lazy var _view: MyView = {
        let myView = MyView()
        self.view.addSubview(myView)
        return myView
    }()

    func cellForRowAtIndexPath(collectionView: UICollectionView) {
        if collectionView = _view.firstCollectionView {        
            ...
        } else {
            collectionView.dequeCell(MyCellIdentifier) 
            ...
        }
    }
}

class MyView: UIView {

    let firstCollectionView = UICollectionView()
    let secondCollectionView = UICollectionView()

    init() {
        secondCollectionView.registerCell(MyCellIdentifier)
    }

}

The app crashed with the error could not dequeue a view of kind: UICollectionElementKindCell with identifier MyCellIdentifier - must register a nib or a class for the identifier

After debugging for a while and trying to understand what happened, I checked the addresses of the collection views by putting a breakpoint in cellForRowAtIndexPath:

collectionview(of cellForRowAtIndexPath): 0x00007f9ae0873200
firstCollectionView = 0x00007f9ae3194200
secondCollectionView = 0x00007f9ae017ac00

To my suprise, the three addresses were different, and I expected to see the collection view address to the same as secondCollectionView's one.

After that, I tried adding an identifier to both the first collection view and the second collection view. After doing that and checking the identifier, I found that the collectionview which was dequeing the cell (parameter of cellForRowAtIndexPath) was firstCollectionView instead of secondCollectionView.

After all of this, I then found out that the lazy UIView initialization closure was being called two times. This was a difficult bug to spot, specially because the crash was on the cell dequeuing.

So what I don't know is why the reference of the collectionview received at cellForRowAtIndexPath was pointing at firstCollectionView (I don't believe it's just coincidence), but still it isn't the point of this post. I'm just trying to show an unexpected bug with lazy var not being threadsafe. I know it's my fault for not thinking about this while implementing it, but I thought it would be relevant to share a practical bug related with not having thread safe lazy vars.

Thanks for reading!

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself feature A feature request or implementation runtime The Swift Runtime
Projects
None yet
Development

No branches or pull requests

5 participants