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: Go 2: allow defer to have its own scope #30130

Closed
vincent-163 opened this issue Feb 8, 2019 · 17 comments
Closed

proposal: Go 2: allow defer to have its own scope #30130

vincent-163 opened this issue Feb 8, 2019 · 17 comments
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Milestone

Comments

@vincent-163
Copy link

vincent-163 commented Feb 8, 2019

EDIT: Added a solution to the problem. Removed the arguments taken from other issues since the deferblock keyword doesn't change these code.

I know this has been proposed many times but I really do find it extremely inconvenient to use defer in the current way. Even if we can't change the current functionality of defer I would suggest that another keyword be introduced with similar functionality.

The nature of defer where all functions are forced to run just before the function returns seems counter-intuitive to me since there are some cases where I would expect the deferred function to run when the resource goes out of to scope instead of when the function returns.

Here are some cases where I find it hard to write code:

  1. Using multiple resources one by one.
    Pseudo code:
lock1.Lock()
if err := operation1(); err != nil {
  lock1.Unlock()
  return err
}
if err := operation2(); err != nil {
  lock1.Unlock()
  return err
}
lock1.Unlock()

lock2.Lock()
if err := operation3(); err != nil {
  lock2.Unlock()
  return err
}
if err := operation4(); err != nil {
  lock2.Unlock()
  return err
}
lock2.Unlock()

The lock1 gets used first, then lock2. If I used defer in this case, I would have to lock lock2 without unlocking lock1, causing potential deadlock and performance issues. In some cases, I want to perform some expensive operations after obtaining the results from operation2(), but if the operation is performed with the lock held it can hurt performance.

  1. Using defer in a for block.
    This is similar to the first issue, except that the resources are wrapped in a for loop. Pseudocode:
for i := 0; i < 10; i++ {
  locks[i].Lock()
  performOperation(i)
  locks[i].Unlock()
}

Without defer Unlock() will not be called in case of a panic(), causing deadlocks. Also all the locks from 0 to 8 would be held while performing operation 9, which will certainly cause deadlocks.

A workaround for the issue is to wrap the defer statement in a closure, like this:

for i := 0; i < 10; i++ {
  func() {
    locks[i].Lock()
    defer locks[i].Unlock()
    performOperation(i)
  }()
}

The problem with this approach is that the closure creates a completely different scope. If we want to break out of the for loop from the closure or return from the function, it would not be possible without some extra code that checks for breaks or returns in the closure. This hurts simplicity.

Intuitively the defer is just like any other blocks, such as for and switch. The deferred function gets called as soon as the program leaves the scope.

I propose a special keyword, called deferblock or something else, that expects to be followed by a block of code, like:

deferblock {
  lock.Lock()
  defer lock.Unlock()
  // perform operations with lock
}

defer always appends a deferred function to the last deferblock rather than an arbitrarily chosen deferblock, since defer functions have to be arranged like a stack and it's unnatural to "insert" to the stack.

deferblock can be break-ed out, just like for, switch and select. When program leaves the deferblock in any way, by normal control flow, break or panic, the deferred functions associated with the scope gets called, and if there's a panic and one of the deferred functions recovers from the panic, the program continues from the point after deferblock.

Such deferblock is consistent with what Go already has, so programmers don't have to rethink about how to use defer.

The new functionality missing from the language is to break out of a deferblock to an outer label, which cannot be easily implemented by using a closure.

@gopherbot gopherbot added this to the Proposal milestone Feb 8, 2019
@ianlancetaylor ianlancetaylor changed the title Proposal: Allow defer() to have its own scope proposal: Go 2: allow defer to have its own scope Feb 9, 2019
@ianlancetaylor ianlancetaylor added the v2 A language change or incompatible library change label Feb 9, 2019
@as
Copy link
Contributor

as commented Feb 11, 2019

The lock1 gets used first, then lock2. If I used defer in this case, I would have to lock lock2 without unlocking lock1, causing potential deadlock and performance issues.

Why does lock1 and lock2 have dependencies on each other? The problem here seems to be a result of misusing sync.

For the other case, how does breaking the function into smaller ones not solve the problem? Theoretical performance impacts are inapplicable for solving problems on a scale measured in nanoseconds.

@vincent-163
Copy link
Author

Why does lock1 and lock2 have dependencies on each other? The problem here seems to be a result of misusing sync.

The idea is that the two locks don't have dependencies, but since defer defers the function call until return, it's not possible to defer Unlock without wrapping it in a closure, which suffers from the other problem (cannot break out of the closure).

For the other case, how does breaking the function into smaller ones not solve the problem? Theoretical performance impacts are inapplicable for solving problems on a scale measured in nanoseconds.

The impact is not in terms of performance but in terms of writing clean code. Wrapping it in a closure forces me to write additional code to handle breaks when break should have done it.

@networkimprov
Copy link

I would use a scope-limited defer.

Combining another keyword or symbol with defer avoids a backwards-incompatible new keyword:

defer{}
defer case
defer range
defer for

You don't need a new block definition method; you can write:

func f() {
   ...
   {
      door.Lock()
      defer{} door.Unlock()
      ...
   }
}

@DeedleFake
Copy link

Or just allow defer to be given a label much like break and continue can be:

lock:
  for {
    m.Lock()
    defer lock, m.Unlock()
  }

Not too keen on the syntax, but it allows you to select a block that way. The question in this case becomes when exactly the defer is run. Does it run when the loop loops or when the loop exits? I can see arguments in either direction.

The more I think about this, the more I think that it's best to just keep the simplicity of the current version, even if it can lead to some occasional awkwardness.

@vincent-163
Copy link
Author

vincent-163 commented Feb 12, 2019

Combining another keyword or symbol with defer avoids a backwards-incompatible new keyword:

Not too keen on the syntax, but it allows you to select a block that way. The question in this case becomes when exactly the defer is run. Does it run when the loop loops or when the loop exits? I can see arguments in either direction.

Thanks for your interest in this proposal.

A highlight in this proposal is that there is no change to the defer syntax; it always puts the deferred function to the direct parent block, and the deferred functions are always called when the code leaves the block in any way. It's just that it's no longer limited to function scope, and people can define their own blocks. The reason, as I've already covered in the first post, is that defer is always a stack and it's unnatural to "insert" to a stack.

I believe this would keep simplicity while encouraging good coding practices. Without a block to contain the defer-ed functions, people would be encouraged to defer everything until function return. Such coding practice causes subtle problems since defer is usually used to free unused resources, so the functions can be called arbitrarily late. People would tend to write:

lock1.Lock()
defer lock1.Unlock()
// operation 1 with lock 1 held

lock2.Lock()
defer lock2.Unlock()
// operation 2 with lock 2 held and lock 1 accidentally held

instead of:

func() {
  lock1.Lock()
  defer lock1.Unlock()
  // operation 1 with lock 1 held
}()

func() {
  lock2.Lock()
  defer lock2.Unlock()
  // operation 2 with lock 2 held
}()

The first piece of code doesn't cause problems until people run into race conditions that are much harder to locate.

By introducing a dedicated keyword for defer blocks, it encourages people to clean up resources as soon as they are out of scope and additionally solves the problem with break-ing out of the closure.

@ianlancetaylor
Copy link
Contributor

I note that you don't actually need a new keyword. Since defer can not be followed by a block, it would suffice to write

defer {
    lock.Lock()
    defer lock.Unlock()
    ...
}

That is, defer followed by a block would mean that any defer statements within the block would execute when the code leaves the block. I don't know if I like this idea, but it doesn't seem to require a new keyword.

@DeedleFake
Copy link

I don't think that would work out well in practice. There would likely be too many times that you would need to do a normal function-scoped defer inside of a defer block. It could also complicate things because you'd have to know if you were inside a defer block or not in order to know what a defer was actually going to do, which could complicate reading other people's code quite a bit. It makes it sound a bit like try {} finally {}, which is something that I, at least, would certainly like to avoid.

In general, I'm not a fan of special blocks of code that change the way syntactically unrelated lines inside the blocks work. It tends towards some annoying to read and maintain code.

@deanveloper
Copy link

The new functionality missing from the language is to break out of a deferblock to an outer label, which cannot be easily implemented by using a closure.

What would this even mean? This seems to be the only advantage of deferblock { ... } over defer func() { ... }().

defer functions are called before a function is exiting, so would breaking out of a defer mean that the function just "stops exiting"? It seems to be a confusing concept. An example where breaking out of a deferblock to an outer label, resulting in clean code, would be very useful.

Until then, this proposal really just seems like syntactic sugar over defer func() { ... }()

@vincent-163
Copy link
Author

What would this even mean? This seems to be the only advantage of deferblock { ... } over defer func() { ... }().

deferblock is not meant to replace defer func(). It surrounds a block of code to call defer methods earlier, so they are called when the program leaves deferblock instead of before function exit.

An example is:

func f() {
  for i := 0; i < 1000; i++ {
    deferblock {
      file := open(i)
      defer file.Close()
      // work with the file
    }
  }
}

The Close happens before the next iteration. This would be useful when the file is only used shortly, so the file doesn't have to wait until function returns before it is closed. With a deferblock it is possible to return directly or break out of the for loop from within the block, which you cannot do with a closure.

It makes it sound a bit like try {} finally {}, which is something that I, at least, would certainly like to avoid.

I personally believe that defer is more like finalizers. People write defer to keep the code that opens a resource and closes a resource together, and to forget about the fact that the resource needs to be closed while writing function body.

In general, I'm not a fan of special blocks of code that change the way syntactically unrelated lines inside the blocks work. It tends towards some annoying to read and maintain code.

This makes sense but I find the defer case special. Think of it like a scope, where variables inside the scope cannot be used outside the scope. A deferblock makes sure that defer functions inside the block are called before the program leaves the scope. It is a bit like how finalizer in C++ works, but IMHO defers are mostly used as finalizers.

There would likely be too many times that you would need to do a normal function-scoped defer inside of a defer block.

When you would need to do this you might not want to use deferblock. Since defer is usually used to execute something arbitrarily late, it's often OK to remove deferblock without serious consequences. I acknowledge that I would seldom use a deferblock since it is often OK to scope defer calls to the function. But there are some rare occasions when I do.

A case that I could imagine you would need to do function-scoped defer is when you want to use an object outside the block, like:

var files [1000]file
for i := 0; i < 1000; i++ {
  deferblock {
    f := openfile(i)
    defer f.Close()
    files[i] := openfile(f.Read())
    defer files[i].Close() // needs to be function-scoped
  }
}
// use files

You need to move the defer outside the deferblock:

var files [1000]file
for i := 0; i < 1000; i++ {
  deferblock {
    f := openfile(i)
    defer f.Close()
    files[i] := openfile(f.Read())
  }
  defer files[i].Close()
}
// use files

Think of this as calling defer at the same scope where you need the files.

@deanveloper
Copy link

deferblock is not meant to replace defer func(). It surrounds a block of code to call defer methods earlier, so they are called when the program leaves deferblock instead of before function exit.

My bad. I made a mistake when I was originally typing. What I meant was func() { ... defer foo() ... }(). I can rewrite all of those with the notation I had originally meant.

func f() {
  for i := 0; i < 1000; i++ {
    func() {
      file := open(i)
      defer file.Close()
      // work with the file
    }()
  }
}

etc etc. I understand the problem, but it seems like a closure would do the same thing, and the only advantage at the higher level is to "break to an outer label," which still doesn't make sense to me as to why you would ever want to do this.

So to revise, it would be good if you could provide an example where a closure would not suffice. Again, my mistake for not typing the original statement correctly.

@vincent-163
Copy link
Author

I understand the problem, but it seems like a closure would do the same thing, and the only advantage at the higher level is to "break to an outer label," which still doesn't make sense to me as to why you would ever want to do this.

A lot of boilerplate code is necessary to make break or return work in a closure, and I believe it makes sense to remove such codes.

Wrapping code in a closure just to execute defer ahead of time is more of a hack than a usual programming pratice, and the nuisance of break and return is enough to drive people away from using closures. That is, people are be more inclined to write:

f1 := open1()
defer f1.Close()
val1, err := f1.Read()
if err != nil {
  return err
}

f2 := open2()
defer f2.Close()
val1, err := f2.Read()
if err != nil {
  return err
}

instead of:

val1, err := func() (value, error) {
  f1 := open1()
  defer f1.Close()
  val1, err := f1.Read()
  if err != nil {
    return value{}, err
  }
  return val1, err
}()
if err != nil {
  return err
}

val2, err := func() (value, error) {
  f2 := open2()
  defer f2.Close()
  val1, err := f2.Read()
  if err != nil {
    return
  }
}
if err != nil {
  return err
}

The first block of code creates an unnecessary dependency as if f2 depended on f1 to work. If f1 and f2 were the same file or lock, it might cause errors or deadlocks. By introducing deferblock people would naturally write the second block of code using deferblock and organize their code in a cleaner way, such that every object lives in a separate scope where possible.

@networkimprov
Copy link

With defer{} instead of deferblock {...}

var v1, v2 int
var err error
{
   f := p.Open(a)
   defer{} f.Close()
   v1, err = f.Read()
   if err != nil {
      return err
   }
} {
   f := p.Open(b)
   defer{} f.Close()
   v2, err = f.Read()
   if err != nil {
      return err
   }
}

@deanveloper
Copy link

deanveloper commented Feb 18, 2019

@HEWENYANG with deferblock, I'll admit it looks nicer, but it's actually one fewer character to do the equivalent of your deferblock { ... } than to do func() { ... }(). It doesn't have any trailing characters though, so that's a big plus.

You also state that closures get a bad rep because of breaking and returning from inside of them. This is very true and I'll admit that much. But I'm not sure if what you have provided is the right solution.

The solution here just doesn't seem very "go-like" to me. It seems like a "oh let's just squeeze this into the language" feature rather than one with a lot of thought and pollish. Honestly maybe just a rename away from deferblock is all that's needed, but it may be a different underlying issue.

@networkimprov this idea has one of the same issues as my previous proposal (mine was just to change defer entirely), since it doesn't account for conditional defer{}s. The syntax itself also isn't very go-like as defer{} looks like it should have something in the brackets rather than being a single token.

I do want to get thinking on a better solution to this problem, though. Encouraging the freeing of resources right when you don't need them anymore is a great thing to do.

This idea may be good? I'm not entirely convinced, but I'll put it here just to add another idea:

var v1, v2 int
var err error
f1 := p.Open(a)
defer f2.Close() {
   v1, err = f2.Read()
   if err != nil {
      return err
   }
}
f2 := p.Open(b)
defer f2.Close() {
   v2, err = f2.Read()
   if err != nil {
      return err
   }
}

@vincent-163
Copy link
Author

vincent-163 commented Feb 18, 2019

@deanveloper That was my original idea. However, another issue mentions this piece of code:

var files []File
for i, filename := range filenames {
  f := open(filename)
  defer f.Close()
  files = append(files, f)
}
// use all files

In this case you really want to use all files at once. This can be rewritten using cheap goroutines to match your idea:

var wg sync.WaitGroup
files := make([]File, len(filenames))
doneChan := make(chan struct{})
for i, filename := range filenames {
  wg.Add(1)
  go func(i int, filename string) {
    files[i] = open(filename)
    wg.Done()
    defer files[i].Close() {
        <-doneChan
    }
  }(i, filename)
}
wg.Wait()
// work with all files
close(doneChan)

The latter piece of code seems more natural to me. Every file should have its own scope and they are independent of each other, so it's natural to spin up a goroutine for each file. But the latter syntax is more complicated, and people are more familiar with the first syntax. It takes a rewrite to switch from the first to the second pattern.

What's more convincing to me is a pattern that I used by myself:

var service1 Service1
if hasService1 {
  service1 = newService1()
  defer service1.Shutdown()
}
var service2 Service2
if hasService2 {
  service2 = newService2(service1)
  defer service2.Shutdown()
}
var service3 Service3
if hasService3 {
  service3 = newService3(service1, service2)
  defer service3.Shutdown()
}

Such "conditional defer" cannot be achieved with block defer syntax. Sometimes it is possible to do wrap the body into another function to achieve conditional defer, but not in this case. It's also natural because it's easy to insert or remove a service anywhere in the code, and it takes advantage of defer's reverse order.

Those arguments led me to the current deferblock idea. I believe this is the simplest way to solve the return and break problem, encourage people to free up resources as early as possible, keep defer intuitive, without significantly changing how defer should be used, so programmers can use their existing skills and not break existing code.

@ianlancetaylor
Copy link
Contributor

Thanks, but this idea doesn't really solve a problem that we have. If defer is blocked, then it cannot be conditional. So you can always simply write the deferred function call at the end of the block. If your block gets so large that this is hard to see, it's probably worth factoring the block into a separate function anyhow.

@pjebs
Copy link
Contributor

pjebs commented Jan 8, 2020

scope based defer would be a beautiful sight.
I created a for-loop scoped defer here if you want to use it: https://github.com/rocketlaunchr/igo

If there is demand for it, I can expand it to all scopes. It's surprisingly easy to do.

@networkimprov
Copy link

@pjebs that's neat! I posted to HN: https://news.ycombinator.com/item?id=21989449

@golang golang locked and limited conversation to collaborators Jan 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

9 participants