Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer in iterators is only guaranteed if the iterator runs to completion #11815

Open
massung opened this issue Jul 22, 2019 · 8 comments
Open

Comments

@massung
Copy link

massung commented Jul 22, 2019

Not sure if this will technically count as a feature request, but in case this not current desired behavior I submitted it as a bug.

When using defer in an iterator, it works only if the iterator runs to completion. I suppose that might be "correct", but it can make some things considerably more tricky.

Consider:

proc count(n: int): iterator(): int =
  return iterator(): int =
    defer: echo "Defer was executed!"
    for i in 1..n:
      yield i

let it = count(10)

for i in 1..5:
  echo $it()

It might be nice if - upon cleanup (the iterator being finalized/garbage collected or program exit) - that the defer still happened. Because - as is - it can make some patterns a bit tricky. Example:

proc readLines(filename: string): iterator(): string =
  var f = open(filename, fmRead)
  return iterator(): string =
    defer: f.close()
    yield $f.readLine()

proc head(filename: string, n: int): seq[string] =
  var nextLine = readLines(filename)
  for i in 1..n:
    let line = nextLine()
    if finished(nextLine):
      break
    result.add(line)

I would have expected head, when done, to close the file. But that won't happen in this case and instead just leave the file open. Yet, once I leave scope, it's impossible for the iterator to advance any more or to ever complete.

I imagine this change might be quite complex given that defer appears to be just a macro that expands to a try..finally?

Anyway, this was something I ran into that took me a bit to track down why some cleanup code wasn't executing. I've worked around it, but - even if the current behavior is kept (and I'd understand why) - I imagine this will be a "gotchya" for future users if not clearly stated somewhere in the documentation.

$ nim -v
Nim Compiler Version 0.20.0 [Windows: amd64]
Compiled at 2019-06-06
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: e7471cebae2a404f3e4239f199f5a0c422484aac
active boot switches: -d:release
@massung massung changed the title Defer in iterators isn't guaranteed Defer in iterators is only guaranteed if the iterator runs to completion Jul 22, 2019
@mratsim
Copy link
Collaborator

mratsim commented Jul 23, 2019

Your iterator is in global scope so it has the lifetime of your program and is never collected.

However this also occurs when the iterator has a shorter lifetime:

proc count(n: int): iterator(): int =
  return iterator(): int =
    defer: echo "Defer was executed!"
    for i in 1..n:
      yield i

proc main() =
  let it = count(10)

  for i in 1..5:
    echo $it()

main()
echo "the end"

@krux02
Copy link
Contributor

krux02 commented Jul 23, 2019

Well, this works

iterator readLines(filename: string): string =
  var f = open(filename, fmRead)
  defer:
    echo "close"
    f.close()
  for line in lines(f):
    yield line

proc head(filename: string, n: int): seq[string] =
  var i = 0
  for line in readLines(filename):
    if i >= n:
      break
    echo "line: ", line
    result.add line
    i += 1

let xxx = head("input", 10)
echo xxx

input-file:

foo
bar
baz
uiatern
iatrnüöäzpqhg
utrnpfghuival
örnüöäp.dnvlgf
pqfhgvlcompuia
irtnptüöänp
wgqflgosíaez.rn
-:)<>>=&)(?)-@
τδρνδϕγψομβζνρτοσε
ℕ∃ΩℝℕΣ⇒↦ΔΦΓ∃ΩΣ
¿4698,65:¡7

output:

line: foo
line: bar
line: baz
line: uiatern
line: iatrnüöäzpqhg
line: utrnpfghuival
line: örnüöäp.dnvlgf
line: pqfhgvlcompuia
line: irtnptüöänp
line: wgqflgosíaez.rn
close
@["foo", "bar", "baz", "uiatern", "iatrnüöäzpqhg", "utrnpfghuival", "örnüöäp.dnvlgf", "pqfhgvlcompuia", "irtnptüöänp", "wgqflgosíaez.rn"]

@massung
Copy link
Author

massung commented Jul 23, 2019

@krux02 - I don't know if that's doing what you think it's doing; you're calling readLines twice, so I would expect to see "close" output twice as well, but it's only output once. I'm not sure which iterator is actually getting the defer called.

@krux02
Copy link
Contributor

krux02 commented Jul 24, 2019

sorry, there was a line that I did not remove properly. I fixed the comment now. I know it is not perfect, but it does work.

@massung
Copy link
Author

massung commented Jul 24, 2019

@krux02 - While I get that your example is showing a situation in which it does work, my example is one in which it does not; yours is a top-level iterator and mine is a closure iterator. Perhaps that makes a difference?

My primary concern is that the behavior of defer in iterator is currently undefined (at best) and broken (at worst). And just suggesting that it either is "fixed" to be guaranteed to execute regardless of the situation (e.g. even if it means waiting until next GC cycle or end of program cleanup) or it be documented that defer will only execute if the function/iterator it is within actually runs to completion and returns.

I'm actually starting to wonder now if spawned functions, threads, and other asynchronous calls with defer blocks won't execute if the program ends before they do?

@krux02
Copy link
Contributor

krux02 commented Jul 25, 2019

Yes, my suggestion was to get it working. That does not mean that there is no bug it should just give you an example to work with and continue your project until the bug has been fixed.

... or it be documented that defer will only execute if the function/iterator it is within actually runs to completion and returns.

I don't think that would be true, as my example shows a defer statement in an iterator that does not run to completion. And here everything works as it should.

@Araq
Copy link
Member

Araq commented Aug 5, 2019

Well, I doubt we can do much about this.

proc count(n: int): iterator(): int =
  return iterator(): int =
    try:
      for i in 1..n:
        yield i
    finally:
      echo "Defer was executed!"

let it = count(10)

for i in 1..11:
  echo $it()

@krux02
Copy link
Contributor

krux02 commented Aug 6, 2019

Then I am tagging it as such. @massung sorry if this causes much headache to you.

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

No branches or pull requests

5 participants