ONT-780 [NCC-2018-014] Add connections limit function. #316

qingche123 · 2018-06-12T03:21:28Z

1.Add total connection count limit function.
2.Add connection count limit function with single ip.

laizy · 2018-06-12T05:56:59Z

p2pserver/net/netserver/netserver.go

@@ -64,6 +64,7 @@ type NetServer struct {
 	ConnectingNodes
 	PeerAddrMap
 	Np *peer.NbrPeers
+	sync.RWMutex


document which field this mutex protect ? only find it is used in Connect function.

I think this block:

connCount := this.GetOutConnectingListLen() if connCount > config.DefConfig.P2PNode.MaxConnOutBound { log.Warnf("Connect: out connections(%d) reach the max limit(%d)", connCount, config.DefConfig.P2PNode.MaxConnOutBound) return errors.New("connect: out connections reach the max limit") }

in Connect function needs to be protected, and I'v changed sync.RWMutex to sync.Mutex

If so, I highly suggest you naming it connectLock instead of embedding struct field.

Honglei-Cong · 2018-06-12T08:53:56Z

p2pserver/net/netserver/netserver.go

+
+//GetPeerSyncCountWithSingleIp return count of cons with single ip
+func (this *NetServer) GetPeerSyncCountWithSingleIp(ip string) uint {
+	this.PeerAddrMap.Lock()


Fixed it, thx

laizy · 2018-06-13T07:00:58Z

p2pserver/net/netserver/netserver.go

+	this.Lock()
+	defer this.Unlock()
+	connCount := this.GetOutConnectingListLen()
+	if connCount > config.DefConfig.P2PNode.MaxConnOutBound {


AFAIK, GetOutConnectingListLen returns the number of nodes in handshaking state, not the total number of connected nodes.

total number of connected nodes, here I mean number of handshaking nodes + number of handshaking completed nodes.

I've fixed it, thx~

Add total connection count limit function. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

lightshine001 · 2018-06-19T03:55:30Z

p2pserver/net/netserver/netserver.go

 	this.PeerConsAddress[addr] = p
 }

 //RemovePeerSyncAddress remove sync addr from peer-addr map
 func (this *NetServer) RemovePeerSyncAddress(addr string) {
-	this.PeerAddrMap.Lock()
-	defer this.PeerAddrMap.Unlock()
+	this.PeerAddrMap.RLock()


Try to change the content, it should be Lock(), not RLock().

lightshine001 · 2018-06-19T03:55:46Z

p2pserver/net/netserver/netserver.go

-	this.PeerAddrMap.Lock()
-	defer this.PeerAddrMap.Unlock()
+	this.PeerAddrMap.RLock()
+	defer this.PeerAddrMap.RUnlock()


Please use Lock()

lightshine001 · 2018-06-19T03:56:00Z

p2pserver/net/netserver/netserver.go

-	this.PeerAddrMap.Lock()
-	defer this.PeerAddrMap.Unlock()
+	this.PeerAddrMap.RLock()
+	defer this.PeerAddrMap.RUnlock()
 	this.PeerSyncAddress[addr] = p


Please use Lock()

lightshine001 · 2018-06-19T03:57:49Z

p2pserver/net/netserver/netserver.go

-	this.ConnectingNodes.Lock()
-	defer this.ConnectingNodes.Unlock()
+	this.ConnectingNodes.RLock()
+	defer this.ConnectingNodes.RUnlock()


Please use Lock()

lightshine001 · 2018-06-19T03:58:11Z

p2pserver/net/netserver/netserver.go

-	defer this.ConnectingNodes.Unlock()
+func (this *NetServer) AddOutConnectingList(addr string) (added bool) {
+	this.ConnectingNodes.RLock()
+	defer this.ConnectingNodes.RUnlock()


lightshine001 · 2018-06-19T03:58:44Z

p2pserver/net/netserver/netserver.go

-	this.PeerAddrMap.Lock()
-	defer this.PeerAddrMap.Unlock()
+	this.PeerAddrMap.RLock()
+	defer this.PeerAddrMap.RUnlock()


lightshine001 · 2018-06-19T03:59:18Z

p2pserver/net/netserver/netserver.go

+
+//AddInConnRecord add in connection to inConnRecord
+func (this *NetServer) AddInConnRecord(addr string) {
+	this.inConnRecord.RLock()


lightshine001 · 2018-06-19T03:59:40Z

p2pserver/net/netserver/netserver.go

+//RemoveInConnRecord remove in connection from inConnRecordList
+func (this *NetServer) RemoveFromInConnRecord(addr string) {
+	this.inConnRecord.RLock()
+	defer this.inConnRecord.RUnlock()


lightshine001 · 2018-06-19T04:00:02Z

p2pserver/net/netserver/netserver.go

+
+//AddOutConnRecord add out connection to outConnRecord
+func (this *NetServer) AddOutConnRecord(addr string, status int) {
+	this.outConnRecord.RLock()


lightshine001 · 2018-06-19T04:00:27Z

p2pserver/net/netserver/netserver.go

+//RemoveOutConnRecord remove out connection from outConnRecord
+func (this *NetServer) RemoveFromOutConnRecord(addr string) {
+	this.outConnRecord.RLock()
+	defer this.outConnRecord.RUnlock()


Add connection count limit function for single ip. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

Remove AddrReq sender from the AddrResp. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

rebrand the connection limit function. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

rebrand single ip connection limit function Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

Add connection limit function with consensus channel. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

Fix RemoveFromInConnRecord bug Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

laizy · 2018-06-19T08:02:39Z

p2pserver/message/utils/msg_handler.go

@@ -49,6 +49,12 @@ func AddrReqHandle(data *msgTypes.MsgPayload, p2p p2p.P2P, pid *evtActor.PID, ar

 	var addrStr []msgCommon.PeerAddr
 	addrStr = p2p.GetNeighborAddrs()
+	for i, peer := range addrStr {
+		if remotePeer.GetID() == peer.ID {
+			addrStr = append(addrStr[:i], addrStr[i+1:]...)


modify array in iteration will cause bug.

Fix AddrReqHandle bug. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

laizy · 2018-06-19T10:39:31Z

p2pserver/net/netserver/netserver.go

+	this.outConnRecord.Lock()
+	defer this.outConnRecord.Unlock()
+	if _, ok := this.outConnRecord.OutConnectingAddrs[addr]; !ok {
+		this.outConnRecord.OutConnectingAddrs[addr] = status


the status stored in OutConnectingAddrs is never be updated properly after set to HAND and used after Connect function. so need updated to track the peer's status or discard to store this like InConnRecord.

laizy · 2018-06-19T10:47:49Z

p2pserver/net/netserver/netserver.go

@@ -269,6 +290,7 @@ func (this *NetServer) Connect(addr string, isConsensus bool) error {
 		}
 		this.RemoveFromConnectingList(addr)


since added == false, this line seems redundant?

if ConnectingList include this addr(AddOutConnectingList return false), but the peer is not exist, then need to do RemoveFromConnectingList

laizy · 2018-06-19T10:52:38Z

p2pserver/net/netserver/netserver.go

@@ -306,6 +330,7 @@ func (this *NetServer) Connect(addr string, isConsensus bool) error {
 		version := msgpack.NewVersion(this, false, ledger.DefLedger.GetCurrentBlockHeight())
 		err := remotePeer.SyncLink.Tx(version)
 		if err != nil {
+			this.RemoveFromOutConnRecord(addr)


missed this line in line 348?

laizy · 2018-06-19T11:12:58Z

p2pserver/net/netserver/netserver.go

@@ -413,7 +437,7 @@ func (this *NetServer) startSyncAccept(listener net.Listener) {
 		conn, err := listener.Accept()
 		if err != nil {
 			log.Error("error accepting ", err.Error())
-			return
+			continue


when Accept returns error, it will return error directly at later call. so need return here to avoid busy loop.

laizy · 2018-06-19T11:14:52Z

p2pserver/net/netserver/netserver.go

@@ -422,13 +446,38 @@ func (this *NetServer) startSyncAccept(listener net.Listener) {
 		log.Info("remote sync node connect with ",
 			conn.RemoteAddr(), conn.LocalAddr())

+		if this.IsAddrInInConnRecord(conn.RemoteAddr().String()) {
+			return


need close connection and continue here.

laizy · 2018-06-19T11:17:02Z

p2pserver/net/netserver/netserver.go

@@ -442,7 +491,7 @@ func (this *NetServer) startConsAccept(listener net.Listener) {
 		conn, err := listener.Accept()
 		if err != nil {
 			log.Error("error accepting ", err.Error())
-			return
+			continue


need return

laizy · 2018-06-19T11:19:25Z

p2pserver/net/netserver/netserver.go

+		if colonPos == -1 {
+			colonPos = len(remoteAddr)
+		}
+		remoteIp := remoteAddr[:colonPos]


why not use common.ParseIPAddr like in line 503?

laizy · 2018-06-19T11:20:05Z

p2pserver/net/netserver/netserver.go

 		if err != nil {
-			log.Errorf("error parse remote ip:%s", addr)
+			log.Error("parse ip error ", err.Error())


close connection

laizy · 2018-06-19T11:20:42Z

p2pserver/net/netserver/netserver.go

+			continue
+		}
+		if !this.IsIPInInConnRecord(remoteIp) {
+			conn.Close()
 			return


need continue

laizy · 2018-06-19T11:22:18Z

p2pserver/net/netserver/netserver.go

@@ -511,8 +574,8 @@ func (this *NetServer) GetPeerFromAddr(addr string) *peer.Peer {
 //IsNbrPeerAddr return result whether the address is under connecting
 func (this *NetServer) IsNbrPeerAddr(addr string, isConsensus bool) bool {
 	var addrNew string
-	this.Np.RLock()
-	defer this.Np.RUnlock()
+	this.Np.Lock()


RLock should be ok here.

Fix some issue in netserver. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

qingche123 force-pushed the master branch from db83923 to da66d20 Compare June 12, 2018 03:24

laizy reviewed Jun 12, 2018

View reviewed changes

Honglei-Cong reviewed Jun 12, 2018

View reviewed changes

qingche123 force-pushed the master branch from 3663944 to b6c3693 Compare June 13, 2018 06:02

laizy reviewed Jun 13, 2018

View reviewed changes

qingche123 force-pushed the master branch 7 times, most recently from 17f13f4 to ac1e329 Compare June 18, 2018 12:53

Add total connection count limit function.

8178741

Add total connection count limit function. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

qingche123 force-pushed the master branch 3 times, most recently from ae1fcdb to fdd27d2 Compare June 19, 2018 02:44

lightshine001 reviewed Jun 19, 2018

View reviewed changes

XiaoJie Guo added 4 commits June 19, 2018 12:44

Add connection count limit function for single ip.

9f46707

Add connection count limit function for single ip. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

Remove AddrReq sender from the AddrResp.

99b95b5

Remove AddrReq sender from the AddrResp. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

rebrand the connection limit function.

65b8feb

rebrand the connection limit function. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

rebrand single ip connection limit function

122005a

rebrand single ip connection limit function Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

XiaoJie Guo added 2 commits June 19, 2018 12:44

Add connection limit function with consensus channel.

6672cfb

Add connection limit function with consensus channel. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

Fix RemoveFromInConnRecord bug

2b18fdf

Fix RemoveFromInConnRecord bug Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

qingche123 force-pushed the master branch from fdd27d2 to 2b18fdf Compare June 19, 2018 04:47

laizy reviewed Jun 19, 2018

View reviewed changes

Fix AddrReqHandle bug.

a7940d2

Fix AddrReqHandle bug. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

qingche123 force-pushed the master branch from 755d236 to a7940d2 Compare June 19, 2018 08:44

laizy requested changes Jun 19, 2018

View reviewed changes

Fix some issue in netserver.

02e28c3

Fix some issue in netserver. Signed-off-by: XiaoJie Guo <guoxiaojie@onchain.com>

qingche123 force-pushed the master branch from dc4d1c1 to 02e28c3 Compare June 20, 2018 03:23

laizy approved these changes Jun 20, 2018

View reviewed changes

laizy merged commit fcee257 into ontio:master Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ONT-780 [NCC-2018-014] Add connections limit function. #316

ONT-780 [NCC-2018-014] Add connections limit function. #316

qingche123 commented Jun 12, 2018

laizy Jun 12, 2018 •

edited

qingche123 Jun 13, 2018

laizy Jun 13, 2018

qingche123 Jun 13, 2018

Honglei-Cong Jun 12, 2018

qingche123 Jun 13, 2018

laizy Jun 13, 2018

laizy Jun 13, 2018

qingche123 Jun 13, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

lightshine001 Jun 19, 2018

laizy Jun 19, 2018 •

edited

laizy Jun 19, 2018

laizy Jun 19, 2018

qingche123 Jun 20, 2018

laizy Jun 19, 2018

laizy Jun 19, 2018

laizy Jun 19, 2018

laizy Jun 19, 2018

laizy Jun 19, 2018

laizy Jun 19, 2018

laizy Jun 19, 2018

laizy Jun 19, 2018

		@@ -269,6 +290,7 @@ func (this *NetServer) Connect(addr string, isConsensus bool) error {
		}
		this.RemoveFromConnectingList(addr)

ONT-780 [NCC-2018-014] Add connections limit function. #316

ONT-780 [NCC-2018-014] Add connections limit function. #316

Conversation

qingche123 commented Jun 12, 2018

laizy Jun 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laizy Jun 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laizy Jun 12, 2018 •

edited

laizy Jun 19, 2018 •

edited