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

Migrate to KCP backend. #44

Merged
merged 34 commits into from Jun 27, 2018
Merged

Migrate to KCP backend. #44

merged 34 commits into from Jun 27, 2018

Conversation

iwasaki-kenta
Copy link
Contributor

@iwasaki-kenta iwasaki-kenta commented Jun 26, 2018

Porting from gRPC to KCP.

TODO:
1. Strange bug that upon peer discovery, the node will Dial() itself. Look into network/discovery/*.go
2. Have the bootstrapping of peers happen only after the server is ready to listen for peers. In other words, block until server is ready in main.go.
3. Figure out how to determine whether or not a peer has properly disconnected exactly once* to remove from the Kademlia peer routing table.
4. Rebase into master and merge.

@iwasaki-kenta iwasaki-kenta added feature New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Jun 26, 2018
@iwasaki-kenta iwasaki-kenta added this to the Release milestone Jun 26, 2018
@iwasaki-kenta iwasaki-kenta requested review from doug-perlin and removed request for doug-perlin June 26, 2018 12:07
return err
}

n, err := ctx.stream.Write(bytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

if client or stream is ever nil, there will be panics here.

Also check if err == nil first before n != len(bytes) so the error returned is more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stream and client are guaranteed to be non-nil as that is the only way we would construct out MessageContext's.

@losfair
Copy link
Contributor

losfair commented Jun 26, 2018

A few questions about this PR:

  • Since KCP is at the same layer with TCP and this PR is just to remove the grpc part and handle raw streams directly (AFAIK), would it better to abstract over the underlying transfer protocol and give user the freedom to pick over TCP or KCP?
  • It seems that sticky/splitted packets aren’t handled?

@iwasaki-kenta
Copy link
Contributor Author

  • Yep, so long as it's compatible with smux (such as traditional TCP in Go) it can easily be abstracted.
  • By splitted packets do you mean data >= 8192 bytes in size? In that case you're right, it would be best not to support it for the time being I believe.

@losfair
Copy link
Contributor

losfair commented Jun 26, 2018

There is also a sticky packet problem where more than one packets ‘stick’ together on one Recv, since TCP and KCP are stream protocols.

@iwasaki-kenta
Copy link
Contributor Author

Mostly solved the issue of sticky packets by using buffered IO.

@iwasaki-kenta
Copy link
Contributor Author

iwasaki-kenta commented Jun 27, 2018

@doug-perlin @abbychau Can you guys look into detecting when peers disconnect? From what I see using KeepAlive in smux would be very useful, and that it can be determined that KeepAlive is broken on the next instance a stream is read/written from.

We need to know when peers are dead to remove them from the routing table of the node.

I'll stop working on this PR for now since everything but disconnection and merging is done 😄.

@doug-perlin
Copy link
Contributor

the disconnection can be a separate pr, this is already pretty big already

@iwasaki-kenta
Copy link
Contributor Author

iwasaki-kenta commented Jun 27, 2018

I'd recommend sticking disconnection in the PR just in case so that it represents one huge transition to KCP. Partially leaving features out would be odd;


// Check if unsigned varint overflows, or if protobuf message is too large.
if n <= 0 || size > 1<<31-1 {
return nil, errors.New("message len is either broken or too large")
Copy link
Contributor

Choose a reason for hiding this comment

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

let's print the size in the error:

return nil, fmt.Errorf("invalid message length: %d bytes", size)

func (n *Network) Listen() {
listener, err := net.Listen("tcp", ":"+strconv.Itoa(n.Port))
listener, err := kcp.ListenWithOptions(":"+strconv.Itoa(int(n.Port)), nil, 10, 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to respect the host too:

kcp.ListenWithOptions(n.Address(), nil, 10, 3)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Best not in case we have other clients wanting to connect through different specific server IP's.

if err != nil {
glog.Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need to glog here, it will get propagated up and may cause a double log.

iwasaki-kenta and others added 21 commits June 27, 2018 10:47
… and a lot of error handling. Still not usable as of yet.
# Conflicts:
#	examples/clusters/messages/cluster.pb.go
@iwasaki-kenta iwasaki-kenta merged commit 158eb0d into master Jun 27, 2018
@iwasaki-kenta iwasaki-kenta deleted the kcp branch June 27, 2018 04:02
@iwasaki-kenta iwasaki-kenta changed the title Migrating to KCP backend. Migrate to KCP backend. Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants