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

Udp net server and client #704 #741

Merged
merged 5 commits into from Mar 31, 2019
Merged

Udp net server and client #704 #741

merged 5 commits into from Mar 31, 2019

Conversation

y0sher
Copy link
Contributor

@y0sher y0sher commented Mar 27, 2019


func (n *UDPNet) Send(to node.Node, data []byte) error {
// todo : handle session if not exist
addr, err := net.ResolveUDPAddr("udp", to.Address())
Copy link
Contributor

Choose a reason for hiding this comment

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

why is there a different resolving algorithm for an address? it's a mapping between nodeid -> ip so should it be the same one as in tcp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no difference. this method is called after we've resolved the IP address by the NodeID.
it's just a resolve method of the IP itself. (so hostnames could be used for example). so this just makes the address string we have about this node to a valid golang UDP address struct.

p2p/net/udp.go Outdated

func (n *UDPNet) listenToUDPNetworkMessages(listener net.PacketConn) {
for {
buf := make([]byte, maxMessageSize) // todo: buffer pool ?
Copy link
Contributor

Choose a reason for hiding this comment

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

why not create it outside the loop?

for {
buf := make([]byte, maxMessageSize) // todo: buffer pool ?
size, addr, err := listener.ReadFrom(buf)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens when there's an error? at least log it

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 code below is what happens when there's an error. it's logged as a warning for Temporary error (e.g reading timeout or other udp possible temp errors). an error log is written and the routine is stopped if there's a critical error in the server.

p2p/net/udp.go Outdated
// todo : check size?
// todo: check if needs session before passing

go func(msg UDPMessageEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if reading from buf failed this code will have errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean there will be no errors but buf will be empty or something like that?

p2p/swarm.go Outdated
@@ -105,7 +107,7 @@ func (s *swarm) waitForGossip() error {
func newSwarm(ctx context.Context, config config.Config, newNode bool, persist bool) (*swarm, error) {

port := config.TCPPort
address := inet.JoinHostPort("0.0.0.0", strconv.Itoa(port))
address := inet.JoinHostPort("127.0.0.1", strconv.Itoa(port))
Copy link
Contributor

Choose a reason for hiding this comment

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

listening on 127.0.0.1 means you are only listening on loopback internal IF' listening on 0.0.0.0 means you are listening on any IF

// todo: no need to return chan, but stay consistent with api
// RegisterDirectProtocolWithChannel registers a protocol on a channel
func (mux *UDPMux) RegisterDirectProtocolWithChannel(name string, c chan service.DirectMessage) chan service.DirectMessage {
mux.messages[name] = c
Copy link
Contributor

Choose a reason for hiding this comment

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

If this method can be called at any time by any routine messages needs to be thread safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this method should be called only right after initialization, before Start. means ProccessDirectProtocolMessage - the routine that reads from the map should never be called concurrently with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment about it

p2p/udp.go Outdated

var data service.Data

if payload := msg.GetPayload(); payload != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if it's neither?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are the only fields we accept as part of this message. if none of them are present it's probably a malformed message. I'll add a treatment for that case.

p2p/net/udp.go Outdated
go func(msg UDPMessageEvent) {

select {
case n.msgChan <- msg:
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 quite understand this, lets talk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Block until msgChan has a free slot (buffered chan) or until shutdown.

msgChan := mux.network.IncomingMessages()
for {
select {
case msg, ok := <-msgChan:
Copy link
Contributor

Choose a reason for hiding this comment

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

this means you are always reading from this channel, when the channel closes, doesn't it return nil to signal it? I'd love to talk about it and see what is the most pragmatic way to do this

Copy link
Contributor Author

@y0sher y0sher Mar 31, 2019

Choose a reason for hiding this comment

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

do you mean it keeps reading after the channel was closed ? either way, it will only happen once. as we return when that's the case.

@@ -84,7 +84,7 @@ endif
.PHONY: $(PLATFORMS)

test:
ulimit -n 400; go test -short -p 1 ./...
ulimit -n 500; go test -short -p 1 ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

wont work on mac default terminal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but now we have more file descriptors since we run a udp server.. (especially this is needed in tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also seems like it works in my mac default terminal

@y0sher y0sher merged commit c588f90 into develop Mar 31, 2019
@y0sher y0sher deleted the udp_net_server branch June 25, 2019 07:43
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.

None yet

2 participants