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.

Sign up for GitHub

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

Jump to bottom

p2p/server: add UDP port mapping goroutine to wait group #20846

Merged
merged 5 commits into from
Apr 1, 2020
Merged

p2p/server: add UDP port mapping goroutine to wait group #20846

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c786a72
p2p/server | add some go routines to wg
ucwong Apr 1, 2020
a2af3d5
p2p/server : not deal with run peer goroutines in mainloop wg
ucwong Apr 1, 2020
f12380f
p2p/server : make sure slot release finally, but main loop wg
ucwong Apr 1, 2020
72a3777
p2p: remove defers
fjl Apr 1, 2020
b7e2867
p2p: fix whitespace
fjl Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions p2p/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,11 @@ func (srv *Server) setupDiscovery() error {
srv.log.Debug("UDP listener up", "addr", realaddr)
if srv.NAT != nil {
if !realaddr.IP.IsLoopback() {
go nat.Map(srv.NAT, srv.quit, "udp", realaddr.Port, realaddr.Port, "ethereum discovery")
srv.loopWG.Add(1)
go func() {
defer srv.loopWG.Done()
nat.Map(srv.NAT, srv.quit, "udp", realaddr.Port, realaddr.Port, "ethereum discovery")
}()
}
}
srv.localnode.SetFallbackUDP(realaddr.Port)
Expand Down Expand Up @@ -666,8 +670,8 @@ func (srv *Server) setupListening() error {
if !tcp.IP.IsLoopback() && srv.NAT != nil {
srv.loopWG.Add(1)
go func() {
defer srv.loopWG.Done()
nat.Map(srv.NAT, srv.quit, "tcp", tcp.Port, tcp.Port, "ethereum p2p")
srv.loopWG.Done()
}()
}
}
Expand Down Expand Up @@ -881,7 +885,9 @@ func (srv *Server) listenLoop() {
fd = newMeteredConn(fd, true, addr)
srv.log.Trace("Accepted connection", "addr", fd.RemoteAddr())
}
srv.loopWG.Add(1)
go func() {
defer srv.loopWG.Done()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also not required because the listenLoop waits for all slots to be returned on exit. Using loopWG here can also lead to concurrent Add and Wait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to deal it in a new independent wg

Copy link
Contributor

Choose a reason for hiding this comment

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

No, because the goroutines are already tracked using the slots channel. Adding a new WaitGroup will not improve it.

Copy link
Contributor

@fjl fjl Apr 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You can track the shutdown of a goroutine in many ways. One way is using sync.WaitGroup. But sometimes WaitGroup is not the best choice.

All the goroutines in p2p.Server are already tracked, except for the UDP port mapping. It's OK to add the UDP port mapping to the loopWG. The other changes are not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh , I see it.
I think it should be better to make sure the slot release finally

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no difference with the defer: if SetupConn causes a panic, the whole process will crash because the panic is not recovered. When the process crashes it does not matter whether the slot is released or not.

srv.SetupConn(fd, inboundConn, nil)
slots <- struct{}{}
}()
Expand Down