Skip to content
This repository has been archived by the owner on Aug 18, 2018. It is now read-only.

go-eicio: Added channel iterator for reader #35

Merged
merged 1 commit into from
Sep 29, 2017
Merged

Conversation

decibelcooper
Copy link
Owner

New method Reader.Events() returns <-chan Event to provide simple syntax
for ranging over all events in a file/stream. Reader.Err was added to
make errors accessible in this scheme.

Additionally, the tools eicio-ls and eicio-strip were updated to iterate events in this way.

This PR resolves #34

New method Reader.Events() returns <-chan Event to provide simple syntax
for ranging over all events in a file/stream.  Reader.Err was added to
make errors accessible in this scheme.
@decibelcooper decibelcooper merged commit c3550a9 into master Sep 29, 2017
@decibelcooper decibelcooper deleted the issue34 branch September 29, 2017 17:02
Copy link

@sbinet sbinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apologies for the belated review.

This type of API is great and does make sense when reading off a (possibly complicated to decode) stream.

but it may lead to leaking goroutines: what happens if the user exits the for evt := range r.Events() loop early?
the goroutine started inside the Events() method will never be collected by the runtime.

I would instead go with a time.NewTicker() API: have a dedicated type that does the looping and put a Close() method on it that can be defer-ed.
That Close() method will then make sure that the "events" reading goroutine will be stopped and collected.

I usually follow the bufio.NewScanner function+type for the naming of such a type:

scan := eicio.NewScanner(reader)
defer scan.Close()

for scan.Scan() {
    evt := scan.Event()
}
if err = scan.Err(); err != nil {
    panic(err)
}

but you could indeed have such an API:

scan := eicio.NewScanner(reader)
defer scan.Close()
for evt := range scan.Events() {
}
if err = scan.Err(); err != nil {
    panic(err)
}

for event, _ := rdr.Get(); event != nil; event, _ = rdr.Get() {
events <- event
}
close(events)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should rather put it as a defer(close(events)) at the top of the goroutine.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good call!

@decibelcooper
Copy link
Owner Author

Thank you for reviewing this!

Indeed, this is an issue that was troublesome for me. In the end I had decided that one should just be aware of this and avoid breaking the loop, but I should have at least put this in the doc comment. However, allowing for such problems is probably bad practice anyway.

I like your solution, though I wish I could find a solution that allows the user code to be slightly more succinct. This problem is one of those Go quirks that gets under my skin a little bit, so I need to give it some thought. I wish that for had a usage that was like the if ASSIGN; BOOL {.

@decibelcooper
Copy link
Owner Author

Perhaps Reader.Events() and Reader.Close() could be modified to clean up any existing event channel?

@sbinet
Copy link

sbinet commented Oct 1, 2017

yes, that would work.
the only difference being that the goroutine would perhaps be stopped a bit later with your scheme.
also: perhaps this will make things like reading some events and then rewinding to the top and reading some more.

but I am probably just nitpicking :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go-eicio: Make channel-based iterator
2 participants