Allow aborting the pack writing operation #24

2013-05-17T04:51:02-05:00

carlosmn commented

2013-05-17 04:51:02 -05:00

(Migrated from github.com)

In case of an error in the writer, the packbuilder will stay around
waiting for someone to read from its channel. The state associated
with a packbuilder is non-trivial and it will keep a reference to the
object, so the GC won't be able to free it.

Change the callback so it knows to abort the writing operation when it
receives a message through the channel and the goroutine can end,
freeing its hold on the packbuilder.

As elegant as the solution with channels is, we should always have a way of telling the producer that we can't consume any more or we won't be webscale.

In case of an error in the writer, the packbuilder will stay around waiting for someone to read from its channel. The state associated with a packbuilder is non-trivial and it will keep a reference to the object, so the GC won't be able to free it. Change the callback so it knows to abort the writing operation when it receives a message through the channel and the goroutine can end, freeing its hold on the packbuilder. --- As elegant as the solution with channels is, we should always have a way of telling the producer that we can't consume any more or we won't be webscale.

vmg commented

2013-05-17 06:30:07 -05:00

(Migrated from github.com)

Ehrm... Why not just call close on the channel?

Ehrm... Why not just call `close` on the channel?

carlosmn commented

2013-05-17 09:07:30 -05:00

(Migrated from github.com)

That's what I wanted to do originally, but that's very un-go-y. Only the producer is meant to close the channel. Closing the channel on the receiving end also means that we would need to willfully panic, catch it in the sender, and then ignore it (because you can only check for closed on the receiving end).

E.g. https://groups.google.com/forum/?fromgroups#!topic/golang-nuts/JB_iiSQkmOk

That's what I wanted to do originally, but that's very un-go-y. Only the producer is meant to close the channel. Closing the channel on the receiving end also means that we would need to willfully panic, catch it in the sender, and then ignore it (because you can only check for closed on the receiving end). E.g. https://groups.google.com/forum/?fromgroups#!topic/golang-nuts/JB_iiSQkmOk

Merovius commented

2013-05-17 09:19:14 -05:00

(Migrated from github.com)

Hm, you are right, I overlooked that. Solution looks good to me.

Merovius commented

2013-05-17 09:27:51 -05:00

(Migrated from github.com)

I think there should be a comment added to ForEach that the channel needs to stay unbuffered, just in case. I think, if someone later would make it buffered, that would introduce a race-condition, if something like the following happens:

Callback writes slice to chan
Write() writes it out, but that takes a while
Meanwhile callback writes another slice
Befofe w.Write() returns, the callback is called again, reads from the channel it's previously written value, assumes Write() encountered an error and wrongly returns

So, because it is not obvious that this could happen, a comment could prevent future pitfalls.

I think there should be a comment added to ForEach that the channel needs to stay unbuffered, just in case. I think, if someone later would make it buffered, that would introduce a race-condition, if something like the following happens: 1. Callback writes slice to chan 2. Write() writes it out, but that takes a while 3. Meanwhile callback writes another slice 4. Befofe w.Write() returns, the callback is called again, reads from the channel it's previously written value, assumes Write() encountered an error and wrongly returns So, because it is not obvious that this could happen, a comment could prevent future pitfalls.

carlosmn commented

2013-05-17 14:27:32 -05:00

(Migrated from github.com)

I've added the comment. Adding buffering is also unnecessarily wasteful of memory, but that's a lesser issue.

carlosmn commented

2013-05-23 04:20:57 -05:00

(Migrated from github.com)

It would be more idiomatic with a second channel that you write into to cancel, wouldn't it? (and much much less fragile) I'll see about implementing that.

Sign in to join this conversation.

No reviewers