Sanitize tests dealing with possible async exceptions

[sinkingsugar]

since the topic of exception safety was raised, this is a school-book example of broken code - on any exception (and there are plenty of them above, even if there's no visible hint of it), this cancel and the close below will not be called - in tests this is annoying because you often get a cascade of failures as ports are not closed correctly, but more generally it's bad because of the proliferation of poor code examples that later are copy-pasted

raising exceptions in nim is very easy and convenient - looking at the outcomes in our code, handling them correctly and consistently turns out to be very hard in practice

Originally posted by @arnetheduck in https://github.com/notifications/beta/MDE4Ok5vdGlmaWNhdGlvblRocmVhZDcwNjE3NjY1Mjo3MDA4OTAw

[zah]

Most of our code is developed in the era before Nim 1.0. Back then, the support for destructors in the language had too many issues to make them practical to use. Nowadays, in Nim 1.0.6 they work well enough if you know where the traps are, but in the current development release of Nim (which will become Nim 1.2 in roughly 2 months from now), many of the issues are further ironed out.

Our plan is to start introducing RAII idioms in the APIs that will address the exception-safety concern with the best practices known from C++. We can start doing this already in Nim 1.0.6 and wait for the improvements in Nim 1.2

[arnetheduck]

transports and futures are ref - refcounting generally is nondeterministic, specially because the async transformation captures references which might lead to.. unusual behavior / cycles. does the two-month timeline include fixing the std lib?

[sinkingsugar]

Well I completely agree, ref and arc are right now being considered like the master solution of all.. imho the original new runtime with just unique pointers and borrow checker was the real nice thing.. not sure what happened to it :(
But also this is a problem of async/await design... with arc/unique real stackful coroutines are the best.

In conclusion:
I personally have quit waiting for nim to become, gotta use what we have now, rest is bonus. I had too many bad experiences doing the opposite!

[zah]

To clarify, the destructors are perfectly usable without switching to arc mode. arc mode determines how strings and sequences are handled.

It's true that the async code relies on ref-counted objects with non-lexically-scoped lifetimes, but this is not quite relevant to the specific issue here, which is "how do we ensure that cancellation happens unless the function completes successfully?". The answer is that we can use a sentry object that executes cancel in its destructor unless it was "defused" right at the end of the function.

[zah]

For the objects with non-deterministic lifetimes, the good news is that the destructors are now properly integrated with the GC and act as finalizers.

[arnetheduck]

issue here is that we want deterministic closing of the transport: the resource must be released before the end of the test and the future cancelled.

in this particular case for example, the async callback will hold a reference to both of these - because we have a global dispatcher, there's also no way to ensure that the global dispatcher queue is cleared of callbacks without explicitly pointing it out (for example manually closing transport and clearing the future)

so perhaps first we need to agree on terminology: these are de facto not destructors but finalizers that sometimes get called deterministically - that emphasizes their non-deterministic nature, and reminds the caller that it is not RAII we're talking about.

[arnetheduck]

a sentry object is again something that manually must be remembered - you can no longer use the transport as it was intended - even if you change the api to do something like let sentry = transport.open(...), how do you ensure that only one copy of the sentry exists deterministically across the hidden closure? if that was "possible" you would not have made transport a ref object in the first place.

in other words, what would this test look like in a world with destructors?

[sinkingsugar]

@arnetheduck indeed on all points... almost sounds like you wish nim had rust standard library 😄 , we were getting close with newruntime unique pointers.. not sure what happened next! maybe was too hard to use...

Well I'm assuming each test must be a Droppable, and on drop/destroy we put all those operations you described above (mostly cos the internal objects we use don't likely support drop.

Another big issue (which @arnetheduck mentioned above too) with imho not so useful GC finalizers is the complete unpredictable (not deterministic) timing on their actual finalize call. That's the reason why .NET even if it has GC had to use IDispose for anything serious.. literally making memory management manual 😄

[mratsim]

AFAIK Arc would be completely deterministic unlike the current GC, cc @Araq

[dryajov]

so perhaps first we need to agree on terminology: these are de facto not destructors but finalizers that sometimes get called deterministically - that emphasizes their non-deterministic nature, and reminds the caller that it is not RAII we're talking about.

This is the point IMO, as it stands we don't really have deterministic object lifetimes and we're stuck with defer and finally for the time being, so unless the new runtime is deterministic and until we switch to it, nothing has really changed with the addition of destructors.