search

deckarep / golang-set

A simple, battle-tested and generic set type for the Go language. Trusted by Docker, 1Password, Ethereum and Hashicorp.
Other
4k stars 272 forks source link

Guidance/suggestions on Iterators and Stop() #130

Open wittekm opened 8 months ago

wittekm commented 8 months ago
  1. If I never call it.Stop() on an Iterator, what happens, does that possibly cause a memory leak? If so, I'd love to have that explicitly called out in documentation.
  2. Is it preferred to do an explicit it.Stop() in a for-loop, or is it usually good enough to just defer it upon iterator creation + use break ?
deckarep commented 8 months ago

Hello,

I think you’re right we could use some clarity around how iterators work and any potential pitfalls.

So in an effort to preserve long-standing backwards compatibility there are actually three ways of doing iteration.

  1. Iter - original method but can leak if the contents of the channel are not fully consumed. ie, an early break or return - at one point this was marked as deprecated and somewhere along the lines it’s been no longer marked that way.
  2. Each - this is just a callback executed per each element to use. You can safely early return, no leaks and no need to close/stop anything.
  3. Iterator - the most recent way - this is an implementation that is more similar to Iterators in the stdlib but does require to call Stop.

I personally recommend Each as I’ve always thought it solves all problems of iterations and doesn’t require messy cleanup at all.

To answer your questions:

  1. When using Iterator you should always call Stop to avoid leaking resources and also provide deterministic cleanup.
  2. Stop should generally be called out of your loop and deferring the call is fine. Remember that defer has function scope, not block scope.
wittekm commented 8 months ago

Cool, thanks for the speedy response. I'd be happy to submit a PR with these recommendations if you'd like (assuming my company is MIT-friendly, pretty sure but gotta double check)

One last question: if you've fully exhausted an Iterator(), do you still need to .Stop() it? If so that's not shown in the iterator example test.

deckarep commented 8 months ago

Calling Stop closes the channel. It’s best practice to do a proper shutdown with things. In the event that the channel is not fully drained Stop will ensure the remaining items are drained.

So at an absolute minimum it closes the channel. It’s not the end of the world as the channel would be garbage collected but you would be future proofing the code if for example someone came back in 3 months and changed the code to early return (or break).

deckarep commented 8 months ago

Also, I’d love a PR that clears up docs. =)