-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Comments
Why does lock1 and lock2 have dependencies on each other? The problem here seems to be a result of misusing 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 idea is that the two locks don't have dependencies, but since
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 |
I would use a scope-limited defer. Combining another keyword or symbol with defer avoids a backwards-incompatible new keyword:
You don't need a new block definition method; you can write:
|
Or just allow 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. |
Thanks for your interest in this proposal. A highlight in this proposal is that there is no change to the I believe this would keep simplicity while encouraging good coding practices. Without a block to contain the 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 |
I note that you don't actually need a new keyword. Since defer {
lock.Lock()
defer lock.Unlock()
...
} That is, |
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 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. |
What would this even mean? This seems to be the only advantage of
Until then, this proposal really just seems like syntactic sugar over |
An example is: func f() {
for i := 0; i < 1000; i++ {
deferblock {
file := open(i)
defer file.Close()
// work with the file
}
}
} The
I personally believe that
This makes sense but I find the
When you would need to do this you might not want to use A case that I could imagine you would need to do function-scoped 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 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 |
My bad. I made a mistake when I was originally typing. What I meant was 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. |
A lot of boilerplate code is necessary to make Wrapping code in a closure just to execute 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 |
With
|
@HEWENYANG with deferblock, I'll admit it looks nicer, but it's actually one fewer character to do the equivalent of your 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 @networkimprov this idea has one of the same issues as my previous proposal (mine was just to change 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:
|
@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 Those arguments led me to the current |
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. |
scope based defer would be a beautiful sight. If there is demand for it, I can expand it to all scopes. It's surprisingly easy to do. |
@pjebs that's neat! I posted to HN: https://news.ycombinator.com/item?id=21989449 |
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 ofdefer
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:
Pseudo code:
The
lock1
gets used first, thenlock2
. 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 fromoperation2()
, but if the operation is performed with the lock held it can hurt performance.for
block.This is similar to the first issue, except that the resources are wrapped in a
for
loop. Pseudocode: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: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
andswitch
. 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:defer
always appends a deferred function to the lastdeferblock
rather than an arbitrarily chosendeferblock
, sincedefer
functions have to be arranged like a stack and it's unnatural to "insert" to the stack.deferblock
can bebreak
-ed out, just likefor
,switch
andselect
. When program leaves thedeferblock
in any way, by normal control flow,break
orpanic
, 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 afterdeferblock
.Such
deferblock
is consistent with what Go already has, so programmers don't have to rethink about how to usedefer
.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.The text was updated successfully, but these errors were encountered: