[Users of NetworkPeer + NetworkPeer] Beyond NetworkPeer refactoring - separate Dispose from Disconnect to avoid cyclic calls

[Aprogiena]

Currently various behaviors call disconnect/dispose on network peer and this leads to cyclic calls like

state changed -> on state changed callback -> dispose peer -> disconnect peer ->state changed to offline -> on state changed callback -> dispose peer

to prevent those, we need to do following steps:

• separate disconnection from disposal (disconnection would only mean doing what is now in NetworkPeerConnection.Disconnect)
• allow disconnection from callbacks but not disposals (this is optional, but a good think to have to see if we fixed those everywhere)
• each component that creates network peer has to manage the list of peers and dispose properly

kind of a big task, which won't be included in NetworkPeer refactoring series

once this is done, we can relax (remove functionality) AsyncQueue and maybe even AsyncExecutionEvent, which are now coded in a way that this cyclic calling is allowed

then we can very much refactor NetworkPeer's Disconnect and NetworkPeerConnection's Shutdown, and Dispose, those could be much much simpler after this is no longer needed

[Aprogiena]

we would maybe want then to introduce OnDisconnect event in NetworkPeer so that those components owning the peer can get notified when this happens (i.e. completely different behavior calls disconnect on their peer) and so they can initiate disposal, but either they would manage themselves that or we would call this OnDisconnect in a non-synchronized fashion, so that we allow disposals from those

[dangershony]

Perhaps we should first refactor that before we go down the odd code pattern in #1079

[Aprogiena]

refactoring of NP is taking care of NP, there is just two things to do and the refactor of NP is finished

#1079 does not add anything new to NP, those cyclic things were there from beginning, we are just making things simpler and simpler but refactoring of NP has its limits

this one touches larger code base, so when NP can't be pushed further, we need to refactor other stuff, which then will allow us to further improve NP stuff

it shouldn't be done in different order because those async events are kind of requirement for this

[noescape00]

I think we can avoid cyclic calls without removing the ability to call Dispose from the callbacks.

So what we can do is that:

1. Save current NetworkPeerState when we start callbacks execution.
2. After each callback is executed check current state against saved state. If they are not equal- stop current execution because callbacks will be updated with newer state.

What's the point of notifying peers about a new state when this state is already outdated?

Fixing this issue that way will make it a small fix so we won't have to redesign the whole thing

[Aprogiena]

I don't think that would be safe to do, I can imagine scenarios where component relies on something being that when the peer is switched to certain state and if you just about the execution it won't happen.

So to me it seems like such a counterintuitive feature of the callback could lead to more problems than it would solve.

And also because the behaviours are not very well designed, I would rather see refactoring that would clean it up

[noescape00]

I don't think that would be safe to do, I can imagine scenarios where component relies on something being that when the peer is switched to certain state and if you just about the execution it won't happen.

I don't think we have behaviors that rely on a certain sequence of states being signaled in order to work correctly.
Though if we want to ensure that we can't jump form one state to a random other we probably should guard against that.

And also because the behaviours are not very well designed, I would rather see refactoring that would clean it up

Ok, I don't mind going with the "big redesign" approach.

I'll think on the best way of approaching that and post new design suggestion here.

[Aprogiena]

I think that is the problem, no one knows. And possibly even bigger problem is that such hidden feature could turn against you in the future.

The refactoring here would not be that much extreme, the number of behaviours we need to attach is reasonable. We only need extreme amount of testing one this is done and we should be fine.

[noescape00]

Suggested changes:

Step 1

NetworkPeerBehavior:

1. Implement protected SubscribeToPayload method to allow peers subscribing for a particular payload or payloads they are interested in instead of letting them subscribe for the whole message.
2. NetworkPeerBehavior should subscribe to AttachedPeer.MessageReceived and when new message is received it will check if derivative class is subscribed to a certain type of payload that was received- if it does- call registered callback.

Benefits:

1. we will get rid of message type checks in all derivitive behaviors
2. we will get rid of copy-pasted implementations of 'AttachCore()' and 'DetachCore()'
3. Code will be cleaner since it will be easy to tell which payloads are handled by the behavior. Also this design suggests and make it easy to handle each payload in a separate method (old design made it easy to handle several payloads in 1 method)

Step 2

NetworkPeerBehavior:

1. Remove DetachCore and Detach.
2. Introduce overridable Dispose() (NetworkPeer will be calling Dispose() for each this.peer.Behaviors)
3. Remove 'RegisterDisposable' - instead just dispose stuff inside the behavior

NetworkPeer:

1. Instead of 'Dispose(string reason, Exception exception = null)', 'Dispose()' and 'Disconnect(string reason, Exception exception = null)' use single "DisconnectAndDispose".
   The reason for that is: we can't disconnect without disposing. After peer is disconnected it's no longer usable so we should do it in a single method.
2. ConnectAsync - change to TryConnectAsync and make it return bool. And since we're disposing the peer in case connection failed we don't actually need the state "Failed".
   Because of the same reason we can remove the 'Offline' state. Since 'Offline' was used as an initial state we will introduce a new state: 'Created' which will be used as an initial state.

3. Rename 'Disconnecting' state to 'Disposed'

NetworkPeerBehavior :
Now since behaviors are being disposed from the peer's dispose code we don't need to update behaviors when state == disposed.
So in NetworkPeerBehavior we will create 2 new virtual methods: OnAttachedPeerHandShaked and OnAttachedPeerConnected. NetworkPeerBehavior will subscribe to peer's StateChanged and if new state is HandShaked\Connected it will call appropriate method.

This will allow the behaviors avoid copy-paste calling of 'AttachedPeer.StateChanged.Register(this.OnStateChangedAsync);' on attach.

Those changes will prevent cyclic calls by design.

[Aprogiena]

I kind of disagree the full step one, this step is unnecessary to fix the issue, it is like completely different thing that is unrelated. Moreover it is already implemented in different class, see network peer listener

[Aprogiena]

Instead of 'Dispose(string reason, Exception exception = null)', 'Dispose()' and 'Disconnect(string reason, Exception exception = null)' use single "DisconnectAndDispose".
The reason for that is: we can't disconnect without disposing. After peer is disconnected it's no longer usable so we should do it in a single method.

I also disagree with this one, we should disconnect separately because it is exactly this that is causing the mess at this moment

As for other suggestions, I will need to look into the code to be able to comment on those

[noescape00]

I also disagree with this one, we should disconnect separately because it is exactly this that is causing the mess at this moment

With those change in place (less states and single DisconnectAndDispose) there won't be place for mess that is currently going on.

When peer is being disposed we will first wait until all behaviors are disposed and after that we start our own disposure.

I'd agree with you if we would be able to do Disconnect and then Connect again. Without that ability Disconnect is 'dispose, but not completely'.

[noescape00]

Moreover it is already implemented in different class, see network peer listener

peer listener implements pull mechanic so in order to use it we would need to implement a loop that would continuously ask for a specific payload. And if we need 2 different payloads we need 2 loops or implement WhenAny logic.

What I want to do is to implement a push mechanic which will make payload processing extremely easy on the user side: create a private method that accepts the payload and register this method on the initialization- that's it.

I kind of disagree the full step one, this step is unnecessary to fix the issue, it is like completely different thing that is unrelated.

It is unrelated to the cyclic calls problem but it is related to the refactoring of the behaviors. As you said:

And also because the behaviours are not very well designed, I would rather see refactoring that would clean it up

[Aprogiena]

I would like to see behaviours refactored, but not as a part of this issue. Let's focus here on removing the cycles and let's keep it minimal.

I will explain on Skype why I don't think that the suggested mechanism of registration is good here