Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix improve transport setup logic issue #818

Merged
merged 1 commit into from
Jun 30, 2021
Merged

Fix improve transport setup logic issue #818

merged 1 commit into from
Jun 30, 2021

Conversation

mrpalide
Copy link
Contributor

Did you run make format && make check?

Fixes #771

Changes:

  • Check transport pk before serving instead after that.

How to test this PR:

@CLAassistant
Copy link

CLAassistant commented Jun 22, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ersonp ersonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as expected. Can see error messages for all 4 transport types if they are not available.

@@ -192,7 +192,7 @@ func (tm *Manager) initTransports(ctx context.Context) {
tpID = entry.Entry.ID
)
isInitiator := tm.n.LocalPK() == entry.Entry.Edges[0]
if _, err := tm.saveTransport(remote, isInitiator, tpType, entry.Entry.Label); err != nil {
if _, err := tm.saveTransport(ctx, remote, isInitiator, tpType, entry.Entry.Label); err != nil {
Copy link
Contributor

@i-hate-nicknames i-hate-nicknames Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I understand correctly, you moved dialing part from SaveTransport to saveTransport. The naming is probably not the best, but SaveTransport is called via rpc and api, and is essentially "I want to add new transport to there".
saveTransport is actually "save this transport and run it". Here it is saved when a transport entry is obtained from transport discovery. Original implementation includes measures to prevent both ends of transport of dialing each other (initially by using PK value and recently by using initiator logic). Currently, it seems that both ends can get into dialing state easily. I'm not sure if this won't introduce weird connectivity issues when a visor comes online with a number of transports for him in TD, while other ends of them are also online.
Can you please elaborate on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The situation that you explain is same as that I have a visor with a served transport (for example a VPN as dmsg), and get offline, and get back online after two hours for example. For now in initTransport, that called when visor get back online, the Serve run again, but without dialing discovery, that means transport(s) added to visor both in online or offline status.

@jdknives jdknives merged commit ca43c3a into skycoin:develop Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants