OC P2P odness #2087

Closed
XFactHD opened this Issue Jan 5, 2016 · 10 comments

Projects

None yet

3 participants

@XFactHD
Collaborator
XFactHD commented Jan 5, 2016

If I try to crash my OC computer with a "no such component" error by disconnecting the me cable in the back of the P2P, the P2P goes offline but the OC network is not disconnected and the OC computer still gets updated data. Any ideas @fnuecke?
AE: rv3 beta 2
OC: 1.5.20

@yueh
Member
yueh commented Jan 5, 2016

Looks like the tunnel is never handling being disabled/removed and updating the OC connection accordingly. They simply create a connection and go to sleep forever.

I do not really know the OC API, so @fnuecke should really look into it.

@yueh yueh added the type-bug label Jan 5, 2016
@fnuecke
Contributor
fnuecke commented Jan 5, 2016

Yeah, I'll have a look. To clarify, the connection gets established via the tunnel, but when the tunnel goes offline, the OC subnets on either side of the tunnel stay connected?

@yueh
Member
yueh commented Jan 5, 2016

You might also want to look into PartP2PTunnel#onTunnelConfigChange() and PartP2PTunnel#onTunnelNetworkChange() to establish the connection instead of ticking the tunnel.

But I cannot really test it as I have no idea how to even use the most basic OC stuff.

@XFactHD
Collaborator
XFactHD commented Jan 5, 2016

@fnuecke What I did was that I connected 21 OC P2P tunnel to my network, then I saved the settings from one P2P and loaded them into the other 20 P2P tunnels. Then I connected all peripherals and the computer and started my program. As I wanted to change a variable and closing the program over touch doesn't work at the moment, I wanted to crash the OC computer with "no such component" by disconnecting the me cable from the P2P tunnel. The P2P tunnel went offline but the computer did still update the information from the peripherals.

@XFactHD
Collaborator
XFactHD commented Jan 5, 2016

@yueh onTunnelNetworkChange is overridden in PartP2POpenComputers.java and contains code to wake the P2P up but onConnect and onDisconnect are overridden and are empty.

@fnuecke
Contributor
fnuecke commented Jan 5, 2016

From what I can remember, the onTunnelNetworkChange schedules a tick because there were some scenarios where reconnecting didn't work properly when updating connections immediately. I'll have another look though, and also at onTunnelConfigChange.

The onConnect and onDisconnect are from OC nodes and should be irrelevant here.

@XFactHD
Collaborator
XFactHD commented Jan 5, 2016

Ah, ok, didn't know they were from OC.

@yueh
Member
yueh commented Jan 5, 2016

Neither of these methods are overridden in PartP2POpenComputers.java

@fnuecke fnuecke added a commit to fnuecke/Applied-Energistics-2 that referenced this issue Jan 5, 2016
@fnuecke fnuecke Reworked computer P2P tunnel to use events, and added check for outpu…
…t port validity, closes #2087.

Instead of just `onTunnelNetworkChange`, this way it is more stable and more exhaustive, avoiding issues with network splits and reconnects not being handled in some cases. Also added check for output port validity when connecting from the perspective of the input, which may potentially have led to invalid connections.
50a9a56
@fnuecke fnuecke added a commit to fnuecke/Applied-Energistics-2 that referenced this issue Jan 5, 2016
@fnuecke fnuecke Reworked computer P2P tunnel to use events, and added check for outpu…
…t port validity, closes #2087.

Instead of just `onTunnelNetworkChange`, this way it is more stable and more exhaustive, avoiding issues with network splits and reconnects not being handled in some cases. Also added check for output port validity when connecting from the perspective of the input, which may potentially have led to invalid connections.
99fc678
@fnuecke fnuecke added a commit to fnuecke/Applied-Energistics-2 that referenced this issue Jan 5, 2016
@fnuecke fnuecke Reworked computer P2P tunnel to use events, and cleaned up reconnecti…
…on, fixes #2087.

Just `onTunnelNetworkChange` with tickable is apparently less exhaustive, and less stable. This now avoiding issues with network splits and reconnects not being handled in some cases. Also simplified reconnection; there was some duplicate logic in there, with a missing validity check which potentially led to invalid connections.
5870214
@fnuecke fnuecke added a commit to fnuecke/Applied-Energistics-2 that referenced this issue Jan 5, 2016
@fnuecke fnuecke Reworked computer P2P tunnel to use events, and cleaned up reconnecti…
…on, fixes #2087.

Just `onTunnelNetworkChange` with tickable is apparently less exhaustive, and less stable. This now avoids issues with network splits and reconnects not being handled in some cases. Also simplified reconnection; there was some duplicate logic in there, with a missing validity check which potentially led to invalid connections.
4fb9625
@fnuecke
Contributor
fnuecke commented Jan 5, 2016

See #2088.

@yueh yueh pushed a commit that closed this issue Jan 8, 2016
@fnuecke fnuecke Reworked computer P2P tunnel to use events, and cleaned up reconnecti…
…on, fixes #2087.

Just `onTunnelNetworkChange` with tickable is apparently less exhaustive, and less stable. This now avoids issues with network splits and reconnects not being handled in some cases. Also simplified reconnection; there was some duplicate logic in there, with a missing validity check which potentially led to invalid connections.
2ea05bc
@yueh yueh closed this in 2ea05bc Jan 8, 2016
@yueh yueh added a commit that referenced this issue Jan 9, 2016
@fnuecke @yueh fnuecke + yueh Reworked computer P2P tunnel to use events, and cleaned up reconnecti…
…on, fixes #2087.

Just `onTunnelNetworkChange` with tickable is apparently less exhaustive, and less stable. This now avoids issues with network splits and reconnects not being handled in some cases. Also simplified reconnection; there was some duplicate logic in there, with a missing validity check which potentially led to invalid connections.
cff5e7b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment