lesismal / llib

BSD 3-Clause "New" or "Revised" License
12 stars 9 forks source link

hacky solution for deadlock #9

Closed acgreek closed 3 years ago

acgreek commented 3 years ago

not a great solution, please clean up

acgreek commented 3 years ago

Yeah, I know this locking code is confusing and bad. I would need to think about it some more if I were to clean it up but it is too late in the day for me; That is why I called it a hacky solution. If you have the cycles; please come up with a better solution

On Mon, Aug 2, 2021, 10:42 PM lesismal @.***> wrote:

@.**** commented on this pull request.

In std/crypto/tls/conn.go https://github.com/lesismal/llib/pull/9#discussion_r681392266:

closed := c.closed

c.closed = true

  • c.closeMux.Unlock()
  • if !c.hasLock {
  1. another goroutine set c.hasLock = false before we get c.hasLock again.
  2. we get a c.hasLock == false and call c.closeMux.Unlock(), without the previos Lock(), then we went wrong.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lesismal/llib/pull/9#pullrequestreview-720731864, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJSWEUMVJN34MXHDTUZ3YTT25JS5ANCNFSM5BNZGO2Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

lesismal commented 3 years ago

Yeah, I know this locking code is confusing and bad. I would need to think about it some more if I were to clean it up but it is too late in the day for me; That is why I called it a hacky solution. If you have the cycles; please come up with a better solution

ok, good night, I'm thinking about it too

lesismal commented 3 years ago

It seems like this: tls.Conn.Write() -> nbio.Conn.Close() -> nbio.Gopher.onCLose() -> tls.Conn.Close() and we get the deadlock here.

Maybe I have get the solusion like this:

// OnClose registers callback for disconnected
func (g *Gopher) OnClose(h func(c *Conn, err error)) {
  if h == nil {
    panic("invalid nil handler")
  }
  g.onClose = func(c *Conn, err error) {
    g.afterFunc(1, func() {
      h(c, err)
    })
  }
}

Execute the nbio.Gopher.onClose in another goroutine, then we should solve it.

lesismal commented 3 years ago

already fixed in nbio.