Skip to content

Conversation

@carlosmn
Copy link
Member

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
Copy link
Member

vmg commented May 17, 2013

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

@carlosmn
Copy link
Member Author

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
Copy link

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

@Merovius
Copy link

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
Copy link
Member Author

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

@carlosmn
Copy link
Member Author

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.

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 ForEach interface to also return a "stop" channel. Closing
the channel or writing into it will cause the first receive clause to
act, making the callback to return -1, aborting the operation and
ending the goroutine, freeing its hold on the packbuilder.
vmg pushed a commit that referenced this pull request Jun 13, 2013
Allow aborting the pack writing operation
@vmg vmg merged commit 01d1a5c into libgit2:master Jun 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants