Rework AsyncIter #811

tbekas · 2024-05-15T16:01:03Z

Separate modules for AsyncIter[T] and Iter[T]
Tests for both asynciter.nim and iter.nim
Helper methods for converting from QueryIter to AsyncIter

codex/indexingstrategy.nim

benbierens

Nice improvements all around.

benbierens · 2024-06-05T08:36:14Z

codex/stores/queryiterhelper.nim

+  ): Future[?!AsyncIter[?!QueryResponse[T]]] {.async.} =
+  ## Converts `QueryIter[T]` to `AsyncIter[?!QueryResponse[T]]` and automatically
+  ## runs dispose whenever `QueryIter` finishes or whenever an error occurs (only
+  ## if the flag finishOnErr is set to true)


There is another case in which a queryIter must be disposed. This is when an iteration is stopped early, before all items have been yielded. LevelDB showed that we were leaking unfinished iters. Sqlite would clean them up automatically on db.close. But LevelDB will throw instead.

I'm not sure this impacts your changes. But please make sure that the QueryIter is disposed correctly, even when the async iter does not error and does not run to completion.

See the TODO here reservations.nim in proc findAvailability:

nim-codex/codex/sales/reservations.nim

Line 629 in 3246c43

# TODO: As soon as we're on ARC-ORC, we can use destructors

Thanks for this comment. I realise that there's a flow like that, but I didn't realise we can address this flow when we have ARC. Current use cases are not finishing iteration prematurely, so either all items have been traversed or an error occurred - both cases are handled.

Yeah it looks in general hard to detect an abandoned iterator unless you can tap into GC hooks. Maybe have an explicit destruct or close operation for query iterators which clients should call if they decide not consume its results anymore? The other option I can think of is a timeout, but that's even trickier I think...

We have this. The query iterator has a dispose you're supposed to call when you're done with the thing. It's done automatically when you complete the iterator and so no one calls dispose. Which is why it started causing problems in this case I linked when levelDB was rolled out. It made the problem visible.
Ideally, when the iterator object is cleaned up, we dispose the iterator handle. The current GC doesn't give us this hook, hence the TODO.

codex/utils/asynciter.nim

codex/utils/iter.nim

benbierens · 2024-06-05T08:49:07Z

tests/codex/stores/testqueryiterhelper.nim

+    tds: TypedDatastore
+
+  setupAll:
+    tds = TypedDatastore.init(SQLiteDatastore.new(Memory).tryGet())


It might be useful to use the LevelDB one, just because it is more picky about disposing iterators. It'll help find issues.
There is no in-memory LevelDB datastore. There's a helper called TempLevelDb that is used in tests for this. Examples everywhere! :D

I prefer such tests to be done in memory only. Maybe there's some in-memory filesystem that we can use for LevelDb?

I haven't looked into this. I don't mind either way. It's just that I noticed using the levelDB for real changes the timing behavior, which has already in the past revealed issues we didn't see. So you can keep the test Sqlite if you want. I'd still suggest you at least run it a few times with the levelDB thing instead, just to see nothing mysterious shows up.

benbierens · 2024-06-05T08:50:34Z

tests/codex/stores/testqueryiterhelper.nim

+  teardownAll:
+    (await tds.close()).tryGet
+
+  test "Should auto-dispose when QueryIter finishes":


If I'm reading this right, this test runs the iter to completion. It'd be really nice to have a test that does not. So it iterates only the first few items of a longer list.

Not sure what such test would suppose to check?

I believe @benbierens wants to guarantee that the query iterator is disposed even if it's not run to completion (which, I think, currently it won't be).

Ok, not sure if that's possible.

Ah yes!
You see, if you use the leveldb helper, then in the setup you create the DB and in teardown you dispose it. If your code does not correctly release the iterators, then your teardown will throw. This way you know there's a problem. (Sqlite doesn't do this, it quietly ignores undisposed iterators.)
So then simply having a test that partially iterates a list would reveal the problem if it's there.

So then simply having a test that partially iterates a list would reveal the problem if it's there.

I mean we know that the iterator will not be disposed in this case. If I would add such test it would always fail on the teardown. Also such test would be impossible to fix currently according to the TODO mentioned in this comment.

We could try to assert that iter isn't disposed in such scenario, but that's even worse, because we would specify (tests are specs) that the expected behavior is not disposing an iterator. Therefore I just prefer to leave it unspecified.

If I'm missing something please provide me an example how such test should look like.

It sounds to me like using the query-iter-helper will guarantee we leak the iterator whenever a query is not run to completion. This can be solved by the other GC in the future. But I suppose in the meantime, we need a way to manually dispose the iter when the user of the iter knows that it's not going to complete.
Either asyncIter has to support manually disposing (for now), or we shouldn't use it instead of queryIter. :| This sucks, but crashes due to leaks or other memory issues are a nightmare to even detect.

Please elaborate on how using this module will guarantee that "we leak the iterator", maybe an example?

gmega

I am puzzled by some things, but maybe because I'm lacking context. I won't block the approval of this PR though as you have enough people asking for things there already. :-)

gmega · 2024-06-05T11:33:42Z

codex/erasure/erasure.nim

@@ -120,7 +120,7 @@ proc getPendingBlocks(
        CatchableError,
        "Future for block id not found, tree cid: " & $manifest.treeCid & ", index: " & $index)

-  Iter.new(genNext, isFinished)
+  newAsyncIter[(?!bt.Block, int)](genNext, isFinished)


Hm... this is sort of different from our style guide. Any reason to have it done like that instead of AsyncIter.new?

The new style will not work well with generics to my knowledge. This call is not going to work: AsyncIter.new[T](...) hence the old style used here and to keep it consistent it's used in both Iter and AsyncIter.

Really? This seems to compile fine here:

type MyGeneric[U, V] = ref object of RootObj u: U v: V proc new*[U, V](t: type MyGeneric[U, V], u: U, v: V): MyGeneric[U, V] = MyGeneric[U, V](u: u, v: v) var a = MyGeneric[int, string].new(1, "hello") echo a.u, " ", a.v

Ok, I haven't tried to use

AsyncIter[T].new(...)

I just tried

AsyncIter.new[T](...)

and that lead to compilation errors. I will the first notation and see if it works.

I used AsyncIter[T].new(...) and it works! Thanks for the suggestion @gmega 👍

codex/erasure/erasure.nim

codex/indexingstrategy.nim

gmega · 2024-06-05T12:04:10Z

codex/stores/queryiterhelper.nim

+  ): Future[?!AsyncIter[?!QueryResponse[T]]] {.async.} =
+  ## Converts `QueryIter[T]` to `AsyncIter[?!QueryResponse[T]]` and automatically
+  ## runs dispose whenever `QueryIter` finishes or whenever an error occurs (only
+  ## if the flag finishOnErr is set to true)


Yeah it looks in general hard to detect an abandoned iterator unless you can tap into GC hooks. Maybe have an explicit destruct or close operation for query iterators which clients should call if they decide not consume its results anymore? The other option I can think of is a timeout, but that's even trickier I think...

codex/utils/asynciter.nim

gmega · 2024-06-05T14:18:35Z

codex/utils/iter.nim

+    isFinished = () => iter.finished
+  )
+
+proc mapFilter*[T, U](iter: Iter[T], mapPredicate: Function[T, Option[U]]): Iter[U] =


Why do we use result objects and handle errors in mapPredicate/iter.next in the async version, but here we don't?

Well there's some handling already provided by newIter, so the iter will finish normally. However it's not perfect (last item before error will get lost I think). It's simply a bit more difficult in non-async version, so I decided to not implement it. I will add it though.

Fixed. Last item before error should no longer get lost.

gmega · 2024-06-05T14:21:46Z

tests/codex/stores/testqueryiterhelper.nim

+  teardownAll:
+    (await tds.close()).tryGet
+
+  test "Should auto-dispose when QueryIter finishes":


I believe @benbierens wants to guarantee that the query iterator is disposed even if it's not run to completion (which, I think, currently it won't be).

gmega · 2024-06-05T14:22:40Z

tests/codex/utils/testasynciter.nim

+    check:
+      items == @[1, 3]
+
+  test "Should leave only odd items using `mapFilter`":


I don't see a test path for the error handling.

In line 79 there's a test that checks if iter finishes on err.

This file only goes until line 77 😂

Probably you look on the outdated version of this file. I added tests for checking errors after @benbierens comments.

gmega

Nothing major on my end so provided you address the rest LGTM.

AuHau

Generally LGTM, I would only like to point out the "catch" of CancelledError being inherited from CatchableError which should be then reraised and not swallowed. For more see Mark's post about it: https://discord.com/channels/895609329053474826/1175098439504244767/1242444718231523338

I think I found one potential place where this should be handled (see the other comment), but generally please have a look on your changes from this POV if maybe there are some other non-handled cases.

AuHau · 2024-06-10T11:42:11Z

codex/utils/asynciter.nim

+          futU.complete(u)
+          nextFutU = some(futU)
+          break
+      except CatchableError as err:


I am not completely sure, but potentially you should check for CancelledError and re-raise it here: https://discord.com/channels/895609329053474826/1175098439504244767/1242444718231523338

Very valueable comment, thanks for pointing it out. I will fix it and add tests to make sure cancellation gets properly propagated.

tests/codex/utils/testasynciter.nim

dryajov · 2024-06-10T17:44:03Z

codex/utils/asynciter.nim

-proc fromSlice*[U, V: Ordinal](_: type Iter, slice: HSlice[U, V]): Iter[U] =
-  ## Creates new iterator from slice
-  ##
+proc new*[U, V: Ordinal](_: type AsyncIter[U], slice: HSlice[U, V]): AsyncIter[U] =


It's not clear anymore what any of this constructors do by themself. Lets add some comments to document it please.

dryajov

LGTM, except for the documentation comment. But we can address in a separate PR.

tbekas force-pushed the asynciter branch 3 times, most recently from 4db0594 to bf84aa2 Compare May 15, 2024 16:46

benbierens assigned tbekas May 15, 2024

dryajov reviewed May 15, 2024

View reviewed changes

codex/indexingstrategy.nim Outdated Show resolved Hide resolved

tbekas force-pushed the asynciter branch 7 times, most recently from fc13503 to 23811a2 Compare May 20, 2024 15:23

tbekas force-pushed the asynciter branch 5 times, most recently from fc555d8 to 9bbe0f6 Compare June 4, 2024 10:00

tbekas marked this pull request as ready for review June 4, 2024 11:59

tbekas requested review from benbierens and gmega June 4, 2024 12:25

benbierens reviewed Jun 5, 2024

View reviewed changes

gmega reviewed Jun 5, 2024

View reviewed changes

tbekas force-pushed the asynciter branch from dbc2d9f to 8944dc6 Compare June 7, 2024 13:10

gmega approved these changes Jun 7, 2024

View reviewed changes

AuHau reviewed Jun 10, 2024

View reviewed changes

benbierens approved these changes Jun 10, 2024

View reviewed changes

tbekas added 4 commits June 10, 2024 17:50

Rework AsyncIter

fc8369a

Add tests for finishing iter on error

c2566a6

Improved error handling for and additional tests

23a3883

Use new style of constructors

86f79fd

Handle future cancellation

7ffe1df

dryajov reviewed Jun 10, 2024

View reviewed changes

dryajov approved these changes Jun 10, 2024

View reviewed changes

AuHau approved these changes Jun 10, 2024

View reviewed changes

Docs for constructors

e009f80

tbekas force-pushed the asynciter branch from dbab73c to e009f80 Compare June 10, 2024 20:03

gmega added this pull request to the merge queue Jun 10, 2024

Merged via the queue into master with commit f51ef52 Jun 10, 2024
10 checks passed

gmega deleted the asynciter branch June 10, 2024 23:48

Rework AsyncIter #811

Rework AsyncIter #811

Conversation

tbekas commented May 15, 2024

benbierens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbekas Jun 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbekas Jun 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmega Jun 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmega left a comment

Choose a reason for hiding this comment

AuHau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov left a comment

Choose a reason for hiding this comment

tbekas Jun 5, 2024 •

edited

tbekas Jun 7, 2024 •

edited

gmega Jun 7, 2024 •

edited