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

Add volatile to InterruptibleLazy valueFactory #17090

Merged
merged 3 commits into from Apr 26, 2024

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Apr 25, 2024

Description

This PR includes the volatile modifier for the valueFactory field. Without this modifier, this code may lead to unexpected situations where, for example, IsValueCreated = true, but Value = null, because of reordering.

NO_RELEASE_NOTES

@DedSec256 DedSec256 requested a review from a team as a code owner April 25, 2024 20:18
Copy link
Contributor

github-actions bot commented Apr 25, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@DedSec256 DedSec256 changed the title Add volatile to InterruptibleLazy valueFactory Add volatile to InterruptibleLazy double-checked value Apr 25, 2024
@DedSec256 DedSec256 changed the title Add volatile to InterruptibleLazy double-checked value Simplify InterruptibleLazy double-checking Apr 25, 2024
@DedSec256 DedSec256 closed this Apr 25, 2024
@DedSec256 DedSec256 reopened this Apr 25, 2024
@DedSec256 DedSec256 force-pushed the ber.a/volatile branch 2 times, most recently from 6360481 to 292b246 Compare April 25, 2024 23:47
@DedSec256 DedSec256 changed the title Simplify InterruptibleLazy double-checking Add volatile to InterruptibleLazy valueFactory Apr 25, 2024
Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

Thanks!

@majocha
Copy link
Contributor

majocha commented Apr 26, 2024

Would a helper holder type like

[<Struct>]
type LazyValue<'T> =
    | Factory of valueFactory: (unit -> 'T)
    | Created of value: 'T

avoid the ordering problem altogether? Though I'm not sure how performant something like this would be:

[<Struct>]
type LazyValue<'T> =
    | Factory of valueFactory: (unit -> 'T)
    | Created of value: 'T

[<Class>]
type InterruptibleLazy<'T> private (lazyValue) =
    let mutable lazyValue = lazyValue

    new(valueFactory: unit -> 'T) = InterruptibleLazy(Factory valueFactory)

    member this.IsValueCreated =
        match lazyValue with
        | Created _ -> true
        | _ -> false

    member this.Value =
        match lazyValue with
        | Created value -> value
        | Factory f ->
            lock this <| fun () ->
                match lazyValue with
                | Created value -> value
                | _ ->
                    try
                        let value = f ()
                        lazyValue <- Created value
                        value
                    with _ ->
                        Unchecked.defaultof<'T>

    member this.Force() = this.Value

    static member FromValue(value) = InterruptibleLazy(Created value)

@DedSec256
Copy link
Contributor Author

DedSec256 commented Apr 26, 2024

@majocha, In the structural DU, the fields of all cases are linearized into a single structure (and in the current example also depend on T). Therefore, updating such a structure will not be atomic and will suffer from struct tearing, which will lead to the following problems in multithreaded access:

  • The order of field initialization is not determined, which may result in the generated compiler DU Tag (which will be checked in pattern matching) being written before Value, so the problem will remain the same as initially.

  • At some point in time, some fields may be overwritten while others may not -- as a result, the structure can be in any incorrect state for reading.

I wouldn't take risks and would opt for a simpler and more reliable option.

@majocha
Copy link
Contributor

majocha commented Apr 26, 2024

Thanks @DedSec256, so, I understand, it would be necessary to lock on all reads and not just writes, unlike current implementation.

@psfinaki psfinaki added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Apr 26, 2024
@psfinaki psfinaki enabled auto-merge (squash) April 26, 2024 14:49
@psfinaki psfinaki merged commit f4bd54c into dotnet:main Apr 26, 2024
32 of 33 checks passed
@DedSec256 DedSec256 deleted the ber.a/volatile branch April 26, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants