Johny b/561 non context manager

[johny-b]

GENERAL NOTES
This resolves #561, but there are follow-up tasks:

• add a warning/error log if the engine wasn't closed gracefully #601
• Add tests for the non-contextmanager Golem interface #605
• Allow to start again a Golem that was stopped #606

THINGS DONE

1. _Engine._start(), _Engine._stop() methods (no logic changes, just extracted code from __aenter__/__aexit__)
2. Golem.operative property
3. Golem.start()/Golem.stop()
4. start() is called in Golem.execute_tasks and Golem.run_service if it wasn't already called before

[johny-b]

Meeting summary:

• Implicit start is fine
• Golem instantiation outside async context is fine
• Remove atexit, because it is not reliable enough, so await golem.stop() is necessary
• Leave context manager as the default mode in examples
• It should be possible to start() a golem that was stop()ped before. Currently something fails.

[shadeofblue]

weren't these methods supposed to be implemented on the _Engine?

[johny-b]

1. All logic is on _Engine.
2. Those are public methods.
   --> I think this is 100% in line with the Twist discussion

[azawlocki]

I've noticed that there's a difference between:

async with Golem(...) as golem:
   do_stuff_with(golem)

and

golem = Golem(...)
await golem.start()
do_stuff_with(golem)
await golem.stop()

in case an exception is raised in golem._start().

With context manager, golem._stop() will be called before re-raising the exception.

With the start()/stop() combo, the developer has to make sure that stop() is called when start() raises an exception. That's fine, if it's intentional and we document this clearly.

[johny-b]

I think that if the developer has any code that ensures golem is stopped when an exception is raised in do_stuff_with(golem) it will also work for start?

E.g. our "recommended" way of doing things without context manager would be

golem = Golem(...)
try:
    do_stuff_with(golem)
finally:
    golem.stop()

(I skipped 'start` because it is optional and I don't think we need it in our examples)

[azawlocki]

@johny-b As I said, that's fine for me. But let's remember about this when creating documentation for start()/stop().

[johny-b]

I think I'll write the docs the way asyncio.Lock is documented, so
"the preferred way is"
"which is equivalent to"

[azawlocki]

Shouldn't we set this to True at the end of the method? If any error occurs before the end of initialization we may end up with a Golem instance that is operative but not properly initialized.

[johny-b]

My reasoning was that we have to guard against more than one start running at the same time, as in:

golem = Golem(...)
loop.create_task(golem.start())
loop.create_task(golem.start())

(I don't think anyone will write exactly this but this might somehow happen in a more complex code, I guess?).

We could store two bits of information, start_started, start_ended.

But maybe we should renameoperative? It is now if fact started_not_stopped. Or maybe just remove a public property?

[azawlocki]

@johny-b You've raised a good point. Consider again your example:

golem = Golem(...)
loop.create_task(golem.start())   # 1
loop.create_task(golem.start())   # 2

Suppose golem.start() in line #1 is in progress and golem.start() in #2 starts. We don't want the second instance of golem._start() to run concurrently with the first one, and your current solution will prevent that. But we also don't want golem.start() in line #2 exiting immediately, as if golem were already properly started, while _start() is still in progress! This is even more important when we replace both golem.start()'s with golem.execute_tasks().

What we do want is for the second golem.start() to wait until the first one finishes. So we need some kind of task synchronisation mechanism, e.g. an asyncio.Lock.

[johny-b]

That's a really good point. I need to think about this : )

[johny-b]

Fixed in 0829514

[azawlocki]

@johny-b Thanks. Now it probably works well, but the solution using asyncio.Futures is a bit hard to follow, at least for me.

Why not something more straightforward, with asyncio.Lock:

def __init__():
    self._started = False
    self._stopped = False
    self._lock = asyncio.Lock()

async def start():
    async with self._lock:
       if not self._started:
           await self._start()
           self._started = True

# Similarly for stop()

@property
def operative():
    return self._started and not self._stopped

EDIT: Perhaps stop() should differ from start() in that self._stopped = True goes before await self.stop(). And there's the complication with the exc_info tuple being passed around from __aexit__().

[azawlocki]

Additionally, if you put start() and stop() in _Engine instead of Golem, you could define _Engine's __aenter__() and __aexit__() in terms of start() and stop(). That would also protect us from concurrent executions of async with golem:... in two different tasks.

[johny-b]

Now I know how to do this, an also after recent master changes it should be quick and easy.
There is a fresh PR: #644