Add Network.remove and auto cleanup #677
Conversation
yapapi/network.py
Outdated
| @@ -89,6 +111,7 @@ async def create( | |||
| # add requestor's own address to the network | |||
| await network.add_owner_address(network.owner_ip) | |||
|
|
|||
| network._state_machine.create() | |||
There was a problem hiding this comment.
This follows the create lifecycle - the first call advances the state machine from initialized to creating, the second one from creating to ready.
The approach taken here is analogous to how the Service lifecycle is defined (ServiceState.lifecycle).
There was a problem hiding this comment.
I think it would be clearer to have two separate, explicit transitions here then -> initialize? that moves from initialized to creating and create that moves from creating to ready
There was a problem hiding this comment.
or maybe create -> creating, start -> ready ? ...
There was a problem hiding this comment.
Updated in b319b73
yapapi/network.py
Outdated
| removed = State("removed") | ||
|
|
||
| # transitions | ||
| add_address = creating.to.itself() | ready.to.itself() |
There was a problem hiding this comment.
hmm... I'm unsure I like how the state transitions are used to control when the methods can be run... I'd expect that logic to be part of those methods and not declared on the state machine ... is this some kind of a well-understood pattern? ... asking since I might not realize that this "transition" is used to control method's usage and rather think that it indeed somehow changes the state of the network...
There was a problem hiding this comment.
Not sure if this is an established pattern, I just found this more elegant.
Let's take add_owner_address as an example. If we wanted to ensure the correct state in the function directly this would look something like this:
if self.state is not NetworkStateMachine.creating or self.state is not NetworkStateMachine.ready:
raise SomeKindOfError("here")
I think that expressing this using possible transitions is cleaner and scales better if we need to handle different situations or new states in the future.
However, if you find this confusing I can think of a different approach.
There was a problem hiding this comment.
nah, maybe it's okay ...
There was a problem hiding this comment.
| add_address = creating.to.itself() | ready.to.itself() | |
| add_owner_address = creating.to.itself() | ready.to.itself() |
There was a problem hiding this comment.
changed in b319b73
yapapi/network.py
Outdated
| @@ -54,6 +55,26 @@ def get_websocket_uri(self, port: int) -> str: | |||
| return f"{net_api_ws}/net/{self.network.network_id}/tcp/{self.ip}/{port}" | |||
|
|
|||
|
|
|||
| class NetworkStateMachine(StateMachine): | |||
There was a problem hiding this comment.
I'd call this class NetworkState for a bit more brevity and to make it consistent with ServiceState
| class NetworkStateMachine(StateMachine): | |
| class NetworkState(StateMachine): |
There was a problem hiding this comment.
Changed in c5d425c
yapapi/network.py
Outdated
|
|
||
| # lifecycle | ||
| create = initialized.to(creating) | creating.to(ready) | ||
| remove = ready.to(removing) | removing.to(removed) |
There was a problem hiding this comment.
and actually, I'd do the same with remove -> have e.g. stop which transitions from ready to removing and remove that moves from removing to removed ...
There was a problem hiding this comment.
Updated in b319b73
Resolves #615