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

proposal: sync: Add Once.Try #53696

Closed
Jorropo opened this issue Jul 6, 2022 · 16 comments
Closed

proposal: sync: Add Once.Try #53696

Jorropo opened this issue Jul 6, 2022 · 16 comments

Comments

@Jorropo
Copy link
Member

Jorropo commented Jul 6, 2022

Pulled out of #53656 (comment)

Add this method to sync.Once:

func (*Once) Try(func() (finished bool))

(not an actual implementation, edge cases are just clearer that way):
When calling Try:

  1. Acquire runner mutex (only one task may run on the once). (defer unlock)
  2. Check if the Once is finished (maybe a task fininshed while we blocked on the mutex)
  • If yes return
  1. Run the passed function
  2. test it's return value:
  • If true mark the Once finished
  1. return

(*sync.Once).Do would keep it's current behaviour.
Meaning a call to Do always succeed and finish the once (even if the function called panic).

Edge cases:

  • What if the current attempt failed but there are other runners queued ? It returns and do not block on them.
  • How do you detect if the current attempt succeeded ?
    Returning a bool value would be poor information, use closures (I expect people to use closures to escape error types).

Implementation details, there would likely be an inlinable fast path using atomic.Load... to quickly return if the once is already finished.

@gopherbot gopherbot added this to the Proposal milestone Jul 6, 2022
@rittneje
Copy link

rittneje commented Jul 6, 2022

I think Try(func() error) error would be a clearer signature that makes it a little less awkward to use.

Also one situation that needs to be considered is if the provided function panics. Today sync.Once.Do considers the action to be completed and subsequent executions are no-ops. However, presumably Try should consider a panic to be a failure. Consequently, your proposal of how to implement Do in terms of Try is not quite correct.

@robpike
Copy link
Contributor

robpike commented Jul 6, 2022

What is the problem you are trying to solve?

@rittneje
Copy link

rittneje commented Jul 6, 2022

@robpike For us, it is fairly common for the function that would have been passed to Once.Do to be capable of failing (e.g., reading some value from a file and caching it forever). In this situation, neither aborting the binary nor saving/returning the original error message forever are desirable solutions. Consequently, we had to write our own version of sync.Once that essentially works as @Jorropo described.

@Jorropo
Copy link
Member Author

Jorropo commented Jul 6, 2022

@robpike I am doing concurrent lazy initialization of shared resources.

However I want the next request to retry a new initialization if the first one failed (like if I'm running out of FDs, or if my connections limits are reached or any IO error that may solve itself).

I have re-implemented a sync.Once like object in my codebase (with a failing behavior similar to this issue), and while discussing #53656 it was suggested that I should use sync.Once instead of writing my own atomic / locking code. (but it doesn't support what I need so here is this issue)

@Jorropo
Copy link
Member Author

Jorropo commented Jul 6, 2022

I think Try(func() error) error would be a clearer signature that makes it a little less awkward to use.

I don't feel strongly about it.
I liked forcing you to use the closure since this nudge you in the more customizable solution.
But this do is very clear.

Consequently, your proposal of how to implement Do in terms of Try is not quite correct.

Ok thx, I have edited my message.

@robpike
Copy link
Contributor

robpike commented Jul 6, 2022

"Concurrent" and "initialization" don't work well together in general, and sync.Once is not the way to address that. In other words, what you want to do may make sense to you in your situation, but in general this is likely to make a hard problem (safe initialization) even harder, and it would be a mistake to add that to the library.

The sync.Once type is fine as is. It solves the problem it was meant to solve.

@earthboundkid
Copy link
Contributor

I also had a situation when I wanted to make some initialization in my app lazy (because it runs on Lambda and cold start is a pain) but the initialization could potentially fail, but once that's the situation, there's no good API (at least that I've seen). The errors have to be dealt with somewhere. (If they could be ignored, regular sync.Once would work.) If the system assumes initialization has already happened, the path to deal with the error isn't there and all you can do is crash. If it doesn't make that assumption, you need to handle the error every time you interact with the object, so it's not really "initialized" just "gettable".

Your proposal is to change sync.Once to sync.AsManyTimesAsItTakes, but that's so different than Once, you can't really reuse the same underlying mechanisms. You need the whole rigamarole of a single goroutine that listens to requests and handles them one at a time until it gets success and then stops. There also needs to be some mechanism for figuring out the retry policy (immediate retries are unlikely to work and likely to make things work). It's just too complicated as an extension to sync.Once, which is really just an optimized atomic.Boolean and a function pointer.

@Jorropo
Copy link
Member Author

Jorropo commented Jul 6, 2022

@carlmjohnson it's very simple, you don't need goroutines or retry logic or anything.
I did say:

  • What if the current attempt failed but there are other runners queued ? It returns and do not block on them.

The implementation is the same as Do except you add a branch on the store (it's a diff just for demonstration):

 func (o *Once) Try(f func() bool) {
 	// Note: Here is an incorrect implementation of Do:
 	//
 	//	if atomic.CompareAndSwapUint32(&o.done, 0, 1) {
 	//		f()
 	//	}
 	//
 	// Do guarantees that when it returns, f has finished.
 	// This implementation would not implement that guarantee:
 	// given two simultaneous calls, the winner of the cas would
 	// call f, and the second would return immediately, without
 	// waiting for the first's call to f to complete.
 	// This is why the slow path falls back to a mutex, and why
 	// the atomic.StoreUint32 must be delayed until after f returns.
 
 	if atomic.LoadUint32(&o.done) == 0 {
 		// Outlined slow-path to allow inlining of the fast-path.
 		o.trySlow(f)
 	}
 }
 
 func (o *Once) trySlow(f func() bool) {
 	o.m.Lock()
 	defer o.m.Unlock()
 	if o.done == 0 {
-		defer atomic.StoreUint32(&o.done, 1)
-		f()
+		if f() {
+			atomic.StoreUint32(&o.done, 1)
+		}
 	}
 }

@earthboundkid
Copy link
Contributor

But you can't use sync.Once.Try to guard a resource because it would race. At that point why not just use atomic.Bool directly?

@Jorropo
Copy link
Member Author

Jorropo commented Jul 6, 2022

@carlmjohnson I don't understand what you are talking about, it still do o.m.Lock() so it guards.

The behaviour I am using is (I assume A fail for example, it's not on purpose):

  • A call Try
    • A's argument is called (wait)
  • B call Try (block because A has mutex)
  • C call Try (block because C has mutex)
  • A fail (A return)
  • B now acquire the lock, B's argument is now called (wait)
  • B's argument succeed (B returns)
  • C acquire the lock, see B already succeed and C return without calling it's argument.
  • D call Try, the Once is already finished and return instantly

In my case most calls don't even overlap, but some rarely do and I need to not crash in thoses cases.

@earthboundkid
Copy link
Contributor

Typical uses of sync.Once guard a resource:

var (
   thing T
   o sync.Once
)

func getThing() T {
   o.Do(func(){
      thing = initialize()
   })
   return thing
}

That would be racey with Try because thing might be modified later if the first initialization fails. It's not safe to touch thing until after the initialization succeeds.

The point of the saying "don't communicate by sharing memory; share memory by communicating" is that these lock/unlock patterns are too complicated to follow and easy to get wrong. Try just makes it even more complicated and hard to follow. It's simpler to just have a goroutine in charge of thing. Ask the goroutine for the thing and it can handle the logic of if there needs to be retry or whatever.

@Jorropo
Copy link
Member Author

Jorropo commented Jul 6, 2022

@carlmjohnson
I don't see anything complicated, if your init error, you just don't touch the shared piece of memory (using @rittneje proposed api because it's better):

var (
   thing T
   o sync.Once
)

func getThing() (T, error) {
   err := o.Try(func() error {
      t, err := initialize()
      if err != nil {
        return err
      }
      thing = t // you could remove the branch up top and always assign and return err also since this part is in the mutex
      return nil
   })
   if err != nil {
     return T{}, err
   }
   return thing, nil
}

@ericchiang
Copy link
Contributor

@Jorropo this sounds like a job for golang.org/x/sync/singleflight. That API is designed for tasks that can produce errors or need to consider context deadlines.

var (
    thing *T
    group singleflight.Group
)

func getThing(ctx context.Context) (*T, error) {
    ch := group.DoChan("", func() (interface{}, error) {
        if thing != nil {
            return thing, nil
        }
        t, err := initialize()
        if err != nil {
            return nil, err
        }
        thing = t
        return t, nil
    })

    select {
    case <-ctx.Done():
        return nil, ctx.Err()
    case r := <-ch:
        if r.Err != nil {
            return nil, t.Err
        } 
        return r.Val.(*T), nil
    }
}

If you're dealing with something that can fail or has to query external resources (e.g. an HTTP request), golang.org/x/sync/... is a good place to start. The "sync" package is more about guarding in-memory operations (like coordinating access to struct fields).

@ianlancetaylor
Copy link
Contributor

sync.Once is less than 100 lines of heavily commented code. It does one job. If you want a different job, I recommend that you copy that code and add the functionality you need.

@ericlagergren
Copy link
Contributor

ericlagergren commented Jul 6, 2022

Here, you can steal this code without adding to the sync package: https://github.com/ericlagergren/lazy

@Jorropo Jorropo closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2022
@Jorropo
Copy link
Member Author

Jorropo commented Jul 7, 2022

@ericlagergren this code doesn't have retry logic.

@golang golang locked and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants