Allow passing net.PacketConn

[AudriusButkevicius]

So the API is still compatible, yet I am able to pass a connection in to perform NAT traversal.

[AudriusButkevicius]

Also the tests should pass...

[xtaci]

https://github.com/xtaci/kcp-go/pull/24/files#diff-801d2c9dc8234c70b6c954d643436df1R105
I think you can't just reference to a fec object. A fec object is stateful and can only be used in only one session.

[codecov-io]

Current coverage is 85.04% (diff: 84.61%)

Merging #24 into master will increase coverage by 0.53%

@@             master        #24   diff @@
==========================================
  Files             6          6          
  Lines          1427       1431     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1206       1217    +11   
+ Misses          171        160    -11   
- Partials         50         54     +4   

Powered by Codecov. Last update 1fb16b3...6a13e8a

[AudriusButkevicius]

What about BlockCrypt?

[AudriusButkevicius]

Judging at the old code, block can be reused.
Addressed the FEC issue.

[xtaci]

newFEC in ListenWithOptions may fail and l.fec may be nil, also accessing member of member is really awful.

[AudriusButkevicius]

I'll add a copy method, and use that

[AudriusButkevicius]

That sadly won't happen for another 2 weeks as I am goijg away on hiliday.

[xtaci]

have a good holiday

[AudriusButkevicius]

Actually, found a few minutes to add it, before I depart.

[xtaci]

what about if I assign them both, then the sequence of the parameters will determine which will be overwritten.

[AudriusButkevicius]

Sure, and why is that wrong?

[xtaci]

the result will be different between the following:

newUDPSession(l, newFEC())

newUDPSession(newFEC(), l)

[AudriusButkevicius]

Sure, but what you'd expect to happen?

[xtaci]

I would prefer a consistent behavior，without worrying about the sequence of parameters.

[AudriusButkevicius]

This method is somewhat internal hence why I think we can allow it to be inconsistent, yet I guess we would then have to limit possibility from the client API.

[xtaci]

I think simplify by taking parameters out of *Listener would be enough.

[AudriusButkevicius]

Yet that doesn't prevent people from providing multiple instances of fec and so on, so I think it's a moot problem we are trying to solve, and people who misuse the api can face the consequences.

[AudriusButkevicius]

So I am back.
What's the latest with this?

[xtaci]

I think raddr & net.PacketConn parameters in Dial function are semantically conflicted,
why not add another API, named DialWithConn? like DialContext in go1.7

[AudriusButkevicius]

No, they are not conflicting, as one is providing where to dial, and the other one provide via what to dial, which is exactly the case here that is needed for stun.

[xtaci]

please make another PR for this, this branch has conflicts.

[AudriusButkevicius]

I can just rebase this PR

[xtaci]

ok cool

[AudriusButkevicius]

rebased

[xtaci]

found another problem:

401 // writeTo wraps write method for client & listener
402 func (s *UDPSession) writeTo(b []byte, addr net.Addr) (int, error) {
403     if s.l == nil {
404         if nc, ok := s.conn.(io.Writer); ok {
405             return nc.Write(b)
406         }
407     }
408     return s.conn.WriteTo(b, addr)
409 }

if you pass in a conn and s.l == nil, how can i tell the conn has Write method?

[AudriusButkevicius]

Took me a while to understand the question. The reason it works in my specific case is because my net.PacketConn has a wrapper that deals with that, but you are right, it won't work in the generic case.

Can we remove the writeTo wrapper and use WriteTo everywhere instead?

[xtaci]

Write/WriteTo has different cost in syscall, Write() in UDP is about 4 times faster than WriteTo(). Low-end router devices benefit a lot from connected UDP.

[AudriusButkevicius]

Sure, so do you have any suggestions?
Golang doesn't provide a way to check if a given packet conn is connected or not. Best I can come up with is checking if the interface satisfies udpconn and check if it has a remote address (which hope is the way you identify if the udpconn is connected or not).

[xtaci]

it's really a hard problem, I think to make a new PacketConn interface may solve this.

[AudriusButkevicius]

As in

ConnectedPacketConn {
net.PacketConn
Connected() bool
}

?

[xtaci]

I mean some wrappers, the caller should bind Write() method to conn.Write() or conn.WriteTo()

[AudriusButkevicius]

Can you elaborate? I am not sure I understand what you mean.

[xtaci]

401 // writeTo wraps write method for client & listener
402 func (s *UDPSession) writeTo(b []byte, addr net.Addr) (int, error) {
403     if s.l == nil {
404         if nc, ok := s.conn.(io.Writer); ok {
405             return nc.Write(b)
406         }
407     }
408     return s.conn.WriteTo(b, addr)
409 }

we do not need this method, we just wrap the connected UDP

WrappedUDPConn implements net.PacketConn {
     WriteTo(b, addr) {
          this.conn.Write(b)   // ignore the addr for connected socket.
    }
}

[AudriusButkevicius]

Oh so you mean do the wrapping internally in the lib for connections we know are connected?

[xtaci]

exactly, callers know this.

[AudriusButkevicius]

Great idea. I'll do this tonight hopefully.

[AudriusButkevicius]

Added the wrapper. Made it public as others might have use-cases for it too.

[xtaci]

good job.
but i'm still not comfort with the API, i'll try some step by step method of add support for conn.

[xtaci]

@AudriusButkevicius see the latest commit:
6a13e8a

NewConn + ServeConn, is this good ?

[AudriusButkevicius]

I thiink yhe API is now much worse, as there are two functions which are not obvious why are they there for, and yet we still have the Options mess convoluted in some functions.

Anyways that will be good enough for my usecase tho.

[xtaci]

opts are mess, i think i 'll remove that.
to clarify the new functions:
NewConn + ServeConn are the real APIs to talk with KCP protocol on a PacketConn

net.UDPConn is a special case of PacketConn. Provided as a default.