io.Closer and closing previously closed object

[ziutek]

io.Closer doc says:

"Closer is the interface that wraps the basic Close method."

It doesn't specify any standard/predictable behavior when Close is
called twice (or more).

Some example:

func aaa() error {
f, err := os.Open("file")
if err != nil {
return err
}
defer f.Close()

if err = bbb(); rr != nil {
return err
}

// More calls that cause return with error, so defer is very useful.

err, ok := ccc()
if err != nil {
return err
}

if ok {
return ddd(f)
}

// We can't complete our work using f and we don't need f any
// more, instead we need to do something with other file/object.
// This can take a long time so we need to close f now to save
// resources.Unfortunately f will be closed again by defered close.

f.Close()

ff, err := os.Open("otherFile")
if err != nil {
return err
}
defer ff.Close()

// Do something with ff (this can take a long time)
}

If f is os.File it is perfectly fine to close previously closed file
(in this case File.Close simply returns an error). But other io.Closer
compatible implementation can for example panic if called twice. This
doesn't conflict with current io.Closer spec.

And... Yes, I known that I can implement aaa function in a way that
avoids double close, but aaa is only an example that explains the
problem.

Does io.Closer spec need to explicitly allow/disallow multiple calls
on the same object?

[minux]

On Tue, Apr 9, 2013 at 4:06 AM, ziutek <ziu...@lnet.pl> wrote:
> io.Closer doc says:
>
> "Closer is the interface that wraps the basic Close method."
>
> It doesn't specify any standard/predictable behavior when Close is
> called twice (or more).

we can't request more here now as it will be incompatible with the
established Go 1 API.
so to be safe, you need to make sure Close method is only called
once.

[Kyle Lemons]

Usually in cases like this I would make an inner, anonymous func that wraps where you do use f.

// stiff before f is opened

if err := func() err {

  f := open(...)

  defer f.Close()

  ...

  logic

  ..

  // done with f

  return nil

}(); err != nil {

  return err

}

// other stuff that doesn't need f

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[ziutek]

> we can't request more here now as it will be incompatible with the
> established Go 1 API.
> so to be safe, you need to make sure Close method is only called
> once.

Ok. I understand that in your opinion future Close spec should contain
something like "you need to make sure Close method is only called
once".

Are there any votes to allow Close to be safely called twice or more
(with returned error, if desired)?

[Jan Mercl]

On Mon, Apr 8, 2013 at 10:28 PM, ziutek <ziu...@lnet.pl> wrote:
> Are there any votes to allow Close to be safely called twice or more
> (with returned error, if desired)?

-1 here. Please define your own IdempotentCloser when/iff needed.

-j

[ziutek]

> -1 here. Please define your own IdempotentCloser when/iff needed.
>
> -j

database/sql sometimes close database driver connection twice. I had
to fix my driver for this case. Should I file an issue instead?

[Brad Fitzpatrick]

yes, but only if you can reproduce it with go1.1beta2.

[Dave Cheney]

Most people ignore the error from close, there isn't much you can do about it. So arguably the second attempt to call Close would return an error, but that error is probably ignored. You can see examples of this in the os and net packages.

[roger peppe]

Personally (Go 1.2?) I'd like to see Close defined as idempotent.
It would be nice if channel close was idempotent too, for conceptual
symmetry, and because it's not uncommon to implement io.Closer using a
channel under the hood.

[ziutek]

My conclusion (inferred from the above answers) is that there is no
safe to call Close twice.

So, we assume that some CORRECT implementation of Close can invalidate
object after first Close and can do strange things (eg. panic) after
second call.

Is my conclusion correct?

[Dave Cheney]

I think your conclusion is logically correct, closing things twice is
at best a noop but more likely an error.

In practice, the way these errors are signaled back to the caller is
via the error value returned from io.Closer.Close().

[ziutek]

> > database/sql sometimes close database driver connection twice. I had
> > to fix my driver for this case. Should I file an issue instead?
>
> yes, but only if you can reproduce it with go1.1beta2.

Sorry, I was wrong. If I can remember correct (but I'm not sure) the
problem was with Rows.Close. I tried to reproduce it with go1.1beta2
without success. So it was fixed or my memory works bad. Sorry!

[peterGo]

On Tuesday, April 9, 2013 2:53:47 AM UTC-4, rog wrote:

Personally (Go 1.2?) I'd like to see Close defined as idempotent.
It would be nice if channel close was idempotent too, for conceptual
symmetry, and because it's not uncommon to implement io.Closer using a
channel under the hood.

+1

Peter

[Jan Mercl]

Closing a channel from multiple goroutines is semantically racy and
thus can make other goroutines crash in that case. Making it
idempotent masks that valuable error detection (the broken is broken).
IMO a very bad idea.

-j

[Jan Mercl]

Ehh: ...the code is broken...

-j

[Dan Kortschak]

'the broken is broken' works there though.

[Jan Mercl]

On Tue, Apr 9, 2013 at 5:03 PM, <gordon...@gmail.com> wrote:
> Code that is racy with non-idempotent channel closing is still racy with
> idempotent channel closing, precisely because close does not synchronize
> with close. That is, unless you assume such synchronization would be
> included along with idempotency, but that seems unlikely.

I don't get the connection to what I wrote. The racy code now panics
if it attempts the second (third....) close. Peter would like it to
not panic. I consider the status quo good for the aforementioned
reason: broken (racy) code panics.

That racy now code is racy with idempotent close is only a tautology.
That's why I'm not sure what are you trying to say.

> I think the point was that it would be nice if a channel close that
> _happens_after_ another close (in the sense defined in the memory model)
> were a no-op.

Still bad IMO. Other goroutines are unaware, so they'll crash
unexpectedly if attempting to write to the closed channel. That again
seems to show that channel close should not happen (w/o panic) more
than once in a correct program.

-j

[ziutek]

I think that on one side defining io.Closer.Close as idempotent close
can simplify some things, but on the other side it can temporary hide
some problems. We can tell the same about some operations allowed on
uninitialized map.

Regardless of which option will be selected, I want to see the clear
spec how io.Closer implementation should work, eg: can it panic on
second close to signal real problem or should it work as noop and
possibly return an error (that is usually ignored).

[Jan Mercl]

On Tue, Apr 9, 2013 at 5:48 PM, ziutek <ziu...@lnet.pl> wrote:
> Regardless of which option will be selected, I want to see the clear
> spec how io.Closer implementation should work, eg: can it panic on
> second close to signal real problem or should it work as noop and
> possibly return an error (that is usually ignored).

That's a bit complicated. There's for instance
http://golang.org/pkg/io/ioutil/#NopCloser. It never returns a non nil
error and does nothing so it is idempotent already. Other
io.Closer(s), e.g. os.File may return non nil errors, and some others
are not idempotent and will panic on "other" closes. So there already
are many "kinds" of them. Specifying only one "kind" will in effect be
not backward compatible.

Go methods match just by signature. Documentation defines the (basic)
intent, but specifying too much details could easily become a problem
as just discussed. Some balance should be IMO found on a case by case
basis.

-j

[Maxim Khitrov]

On Tue, Apr 9, 2013 at 12:11 PM, Jan Mercl <0xj...@gmail.com> wrote:
> On Tue, Apr 9, 2013 at 5:48 PM, ziutek <ziu...@lnet.pl> wrote:
>> Regardless of which option will be selected, I want to see the clear
>> spec how io.Closer implementation should work, eg: can it panic on
>> second close to signal real problem or should it work as noop and
>> possibly return an error (that is usually ignored).
>
> That's a bit complicated. There's for instance
> http://golang.org/pkg/io/ioutil/#NopCloser. It never returns a non nil
> error and does nothing so it is idempotent already. Other
> io.Closer(s), e.g. os.File may return non nil errors, and some others
> are not idempotent and will panic on "other" closes. So there already
> are many "kinds" of them. Specifying only one "kind" will in effect be
> not backward compatible.

I think adding a single sentence along the lines of "unless noted
otherwise, it is not safe to call Close multiple times" would be
sufficient to clear up any confusion. It's just documenting existing
behavior.

[ziutek]

On 9 Kwi, 18:11, Jan Mercl <0xj...@gmail.com> wrote:
> That's a bit complicated. There's for instancehttp://golang.org/pkg/io/ioutil/#NopCloser. It never returns a non nil

> error and does nothing so it is idempotent already. Other
> io.Closer(s), e.g. os.File may return non nil errors, and some others
> are not idempotent and will panic on "other" closes. So there already
> are many "kinds" of them. Specifying only one "kind" will in effect be
> not backward compatible.
>
> Go methods match just by signature. Documentation defines the (basic)
> intent, but specifying too much details could easily become a problem
> as just discussed. Some balance should be IMO found on a case by case
> basis.
>
> -j

If some correct implementation of io.Closer can panic on second call
(or do more strange things), we should assume that any correct
implementation can. So spec should clearly say that you can call Close
only once. You still can implement io.Closer as idempotent close
because it is compatible with such more restricted spec.

[Brad Fitzpatrick]

Agreed.  Done:  https://code.google.com/p/go/source/detail?r=cad0d518446

[Jan Mercl]

https://code.google.com/p/go/source/detail?r=cad0d5184466

-j

[Jan Mercl]

On Tue, Apr 9, 2013 at 11:11 PM, <gordon...@gmail.com> wrote:
> It might be worth noting that the current behavior (panic on multiple closes
> between unsynchronized goroutines) is not guaranteed by the spec.

Quoting from: http://golang.org/ref/spec#Close

"Sending to or closing a closed channel causes a run-time panic."

-j

[Gordon Klaus]

You missed the key phrase "between unsynchronized goroutines".  If there is no synchronization, then a channel close cannot be guaranteed to be observed.

[John Nagle]

On 4/8/2013 1:16 PM, Kyle Lemons wrote:
> Usually in cases like this I would make an inner, anonymous func that wraps
> where you do use f.
>
> // stiff before f is opened
>
> if err := func() err {
> f := open(...)
> defer f.Close()

For write operations, you must always check the status on
"close", because errors on buffered I/O may not be reported
until file close.

John Nagle

[Jan Mercl]

On Wed, Apr 10, 2013 at 12:21 AM, Gordon Klaus <gordon...@gmail.com> wrote:
> You missed the key phrase "between unsynchronized goroutines". If there is
> no synchronization, then a channel close cannot be guaranteed to be
> observed.

I don't think so. Channel is implemented as a pointer, so the memory
model allows a variable of type channel to be seen differently by two
non syncing goroutines. But as long as those two (or any number of)
goroutines already have the same pointer to the run time object
implementing a channel - it is no more possible after a channel close
- for any goroutine which attempts to close the same channel again, to
see it as non closed. By extension: No non syncing goroutine can see a
sync.Mutex as unlocked after it was locked by any other non syncing
goroutine (let's assume no Unlock ever happens to make it simple).
Channels (and eg. mutexes) are intrinsically safe for concurrent
access - the variables (arguments, pointers to) are not - if being
updated by a non syncing goroutine.

IOW: The memory model domain covers _values_ of variables and
arguments (and things possibly pointed to by them), not necessarily
the (internal, opaque) _state_ of run time objects.

-j

[roger peppe]

On 9 April 2013 13:51, Jan Mercl <0xj...@gmail.com> wrote:
> On Tue, Apr 9, 2013 at 2:44 PM, peterGo <go.pe...@gmail.com> wrote:
>> On Tuesday, April 9, 2013 2:53:47 AM UTC-4, rog wrote:
>>>
>>> Personally (Go 1.2?) I'd like to see Close defined as idempotent.
>>> It would be nice if channel close was idempotent too, for conceptual
>>> symmetry, and because it's not uncommon to implement io.Closer using a
>>> channel under the hood.
>
> Closing a channel from multiple goroutines is semantically racy and
> thus can make other goroutines crash in that case.

It's not inherently racy IMHO. At several points in the past, I've had the
situation where I know in the code that nothing more can be written
to a channel, and I need to ensure that it's closed, but other goroutines
are doing the same thing. I've needed to add a mutex just to prevent
calling close twice.

> Making it idempotent masks that valuable error detection (the broken is broken).
> IMO a very bad idea.

If there's a race between a close and a write (that's where the real
problems lie)
we'll get a panic as we should.

[Gordon Klaus]

On Wed, Apr 10, 2013 at 3:32 AM, Jan Mercl <0xj...@gmail.com> wrote:

On Wed, Apr 10, 2013 at 12:21 AM, Gordon Klaus <gordon...@gmail.com> wrote:
> You missed the key phrase "between unsynchronized goroutines".  If there is
> no synchronization, then a channel close cannot be guaranteed to be
> observed.

I don't think so. Channel is implemented as a pointer, so the memory
model allows a variable of type channel to be seen differently by two
non syncing goroutines. But as long as those two (or any number of)
goroutines already have the same pointer to the run time object
implementing a channel - it is no more possible after a channel close
- for any goroutine which attempts to close the same channel again, to
see it as non closed.

You are probably right about the current implementation, and from a practical standpoint this is of course important.  However, the statements I made were implementation-independent, i.e., about the language in its purest form, as defined by the spec and memory model.  It is allowed to have an implementation (e.g., on a distributed memory architecture) in which different goroutines have different copies of the same channel; and in which these copies are synchronized only to satisfy the happens-before relationships defined in the memory model:  send-receive and close-receive, but not close-close or anything else.

 

By extension: No non syncing goroutine can see a
sync.Mutex as unlocked after it was locked by any other non syncing
goroutine (let's assume no Unlock ever happens to make it simple).

This is true (although not by extension of the above) because otherwise the mutex would be broken.  It is guaranteed by the memory model.  From http://golang.org/ref/mem#tmp_7:  "For any sync.Mutex or sync.RWMutex variable l and n < m, call n of l.Unlock() happens before call m of l.Lock() returns."  So if there is no call to l.Unlock(), then only the first call to l.Lock() can return; i.e., subsequent calls to l.Lock() will "see" that it is locked.

 

Channels (and eg. mutexes) are intrinsically safe for concurrent
access - the variables (arguments, pointers to) are not - if being
updated by a non syncing goroutine.

Channel operations are safe, yes.  But that doesn't mean they have to be synchronized in all cases.  For example, channel sends from multiple goroutines certainly don't have to synchronize with each other -- what matters is that a receive synchronizes with a send.

IOW: The memory model domain covers _values_ of variables and
arguments (and things possibly pointed to by them), not necessarily
the (internal, opaque) _state_ of run time objects.

You're right.  The language makes no guarantees about the internal state of runtime objects, only their observable behavior.  This means our discussion of "seeing a lock as unlocked" or "seeing a channel as closed" doesn't always strictly make sense, or it is implementation-dependent.  It makes more sense to discuss the observable order and effect of operations.  And when it comes to multiple channel closes on unsynchronized goroutines, there is nothing to say about their guaranteed observable order or effect on each other except with respect to a particular implementation.

-j

[Gordon Klaus]

On Wed, Apr 10, 2013 at 1:18 PM, Gordon Klaus <gordon...@gmail.com> wrote:

By extension: No non syncing goroutine can see a
sync.Mutex as unlocked after it was locked by any other non syncing
goroutine (let's assume no Unlock ever happens to make it simple).

This is true (although not by extension of the above) because otherwise the mutex would be broken.

I should have said "...(although not [necessarily] by extension of the above)...".

 

Channels (and eg. mutexes) are intrinsically safe for concurrent
access - the variables (arguments, pointers to) are not - if being
updated by a non syncing goroutine.

Channel operations are safe, yes.  But that doesn't mean they have to be synchronized in all cases.  For example, channel sends from multiple goroutines certainly don't have to synchronize with each other -- what matters is that a receive synchronizes with a send.

Actually, I take back that channel operations are safe... I think.  They are apparently safe in the current implementation (although they may panic).  However, in general (i.e. in an arbitrary conforming implementation), sending to a channel that was closed on an unsychronized goroutine is a race condition (and thus has undefined behavior).  And so is closing such a closed channel, although it seems like this could be changed.

But I would appreciate it if someone more knowledgeable weighed in on this.  Ian?

[Ian Lance Taylor]

On Wed, Apr 10, 2013 at 10:47 AM, Gordon Klaus <gordon...@gmail.com> wrote:
>
> Actually, I take back that channel operations are safe... I think. They are
> apparently safe in the current implementation (although they may panic).
> However, in general (i.e. in an arbitrary conforming implementation),
> sending to a channel that was closed on an unsychronized goroutine is a race
> condition (and thus has undefined behavior). And so is closing such a
> closed channel, although it seems like this could be changed.
> But I would appreciate it if someone more knowledgeable weighed in on this.
> Ian?

I can't claim to have understood this whole thread, but I agree that
if there are two unsynchronized goroutines, and one sends on a channel
and the other closes the channel, then that is a race. And I agree
that if two unsynchronized goroutines close a channel, that is a race
too, and that it would be possible to change the language to permit
this case.

Ian

[John Nagle]

Currently, in Go, closing a closed channel or sending on a closed
channel causes a panic. This is preferable to silently failing.

Firefox has an internal channel implementation used between
add-ons and content scripts. Sending to a nonexistent or closed
channel silently fails. This results in hard to find bugs.
Don't go there.

John Nagle

[Kyle Lemons]

The panic behavior is currently guaranteed by the spec.  However, it doesn't specify *when* the panic will happen, so in some implementation where optimizations are made, it may happen at the runtime's leisure.

 

                                John Nagle

[Gordon Klaus]

It definitely makes sense to fail loudly when sending on a closed channel; the sender's expectation that his data will be transmitted is violated.

But closing a closed channel seems pretty harmless (just like deleting a non-existent key from a map); the sender's expectation that the receiver is notified of closure is still satisfied.

                                John Nagle



--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/LJPi2RYI4c8/unsubscribe?hl=en-US.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.

[Gordon Klaus]

On Wed, Apr 10, 2013 at 5:57 PM, Kyle Lemons <kev...@google.com> wrote:

The panic behavior is currently guaranteed by the spec.

The spec says that closing or sending on a closed channel will panic.  But an unsynchronized goroutine isn't guaranteed to observe the closing of a channel, and is therefore not guaranteed to panic.

 

However, it doesn't specify *when* the panic will happen, so in some implementation where optimizations are made, it may happen at the runtime's leisure.

None of the runtime panics in the specification indicate *when*.  I don't think that means they may all occur at the runtime's leisure -- which could be never.