go-pg / pg

Golang ORM with focus on PostgreSQL features and performance
https://pg.uptrace.dev/
BSD 2-Clause "Simplified" License
5.65k stars 401 forks source link

Update err for single conn pool when context.Done() return first #1981

Closed xin-tsla closed 1 year ago

xin-tsla commented 1 year ago

Hi All, The issue this PR tries to solve is that we observed an NPE in OnConnect method https://github.com/go-pg/pg/blob/f7e1c98fed3044ae4e195a0e063f69185fb1885b/base.go#L119 when we call conn.Exec() twice inside the OnConnect method. and the second call (conn.Exec()) may get an nil conn object and fails to call conn.Exec()[panic].

The data flow diagram depicts how the conn is nil in some case: ctxcancelledsingleconnpool drawio

If ctx gets cancelled and returns before query finishes (Yellow line in the diagram), the conn in SingleConnPool will call Remove method to set the conn to nil: https://github.com/go-pg/pg/blob/f7e1c98fed3044ae4e195a0e063f69185fb1885b/internal/pool/pool_single.go#L38 and as well to set an error with reason, which is nil since it is due to ctx gets cancelled without an error: https://github.com/go-pg/pg/blob/f7e1c98fed3044ae4e195a0e063f69185fb1885b/internal/pool/pool_single.go#L39

When ctx is cancelled: https://github.com/go-pg/pg/blob/f7e1c98fed3044ae4e195a0e063f69185fb1885b/base.go#L153 and send struct{}{} to fnDone: https://github.com/go-pg/pg/blob/f7e1c98fed3044ae4e195a0e063f69185fb1885b/base.go#L159

xin-tsla commented 1 year ago

Hi @elliotcourant , wondering when you have time, could you take a second look?

elliotcourant commented 1 year ago

Thank you very much for the fix @xin-tsla !

xin-tsla commented 1 year ago

@elliotcourant , happy to work on the fix! And much appreciate your review and time! :)

xin-tsla commented 1 year ago

@elliotcourant , when you have time, another question, could you create a new release version?

elliotcourant commented 1 year ago

I can tag it for release this week, gonna try to loop in some other changes with that release as well. Mostly dependency bumps.

xin-tsla commented 1 year ago

@elliotcourant , got it. Thank you so much for tagging the release! :)

xin-tsla commented 1 year ago

@elliotcourant , by chance when you have time, could you tag a new release version?

elliotcourant commented 1 year ago

@xin-tsla Sorry for the delay! It is now tagged!

xin-tsla commented 1 year ago

@elliotcourant , no worries! Thanks for tagging it! Much appreciate your time!