# Pool lock - Why we should avoid concurrent connections in a goroutine?
## Problems
In the master code, if we run the integration tests, there would be maximum of 24 concurrent connections, and 23 connection leakages in the end.

<img width="397" alt="Screen Shot 2022-11-29 at 1 00 14 AM" src="https://user-images.githubusercontent.com/104896799/204337280-2e62cd90-c016-4baf-9377-676a053734fd.png">

After addressing the problems described in this article concurrent connections went down to 2, and there is not any connection leakage at the end.

<img width="397" alt="Screen Shot 2022-11-29 at 12 47 26 AM" src="https://user-images.githubusercontent.com/104896799/204334646-dcc6e0c7-2fca-4ba9-b1a6-84ee8a8493db.png">

We can even set the maximum open connection to 1 without deadlock.

<img width="397" alt="Screen Shot 2022-11-29 at 12 31 37 PM" src="https://user-images.githubusercontent.com/104896799/204439393-e8a5ab8b-0d3a-4d28-a177-4e68909da60a.png">

## Why we should avoid concurrent connections in a goroutine
### What's pool locking?
You can see a formula [here](https://github.com/brettwooldridge/HikariCP/wiki/About-Pool-Sizing#pool-locking) to avoid pool locking.
$pool size = Tn \times (Cm - 1) + 1$
* Tn is the maximum goroutines we would run in our server.
* Cm is the maximum of concurrent connections in a goroutine.

For example, if we have two goroutines `Ga` and `Gb`, and maximum of the concurrent connections in a handler is 2, then we need $2 \times (2 - 1) + 1 = 3$

What if we only allow two connections in the pool.
1. `Ga` acquires one connection.
2. `Gb` acquires one connection.
3. `Ga` waits the connection released from `Gb`
4. `Gb` waits the connection released from `Ga`
5. Dead Lock

If we have one additonal connection resource, then either `Ga` and `Gb` can finish their processing and release the connection back to pool.

If we check the formula, we can see that if we only use one connection at a time, then only $Tn \times (1-1) + 1 = 1$ connection is required to avoid deadlock, we can successfully handle the spike traffic without dead lock.

On the other hand, if we have two connections at a time, then $Tn \times (2 - 1) + 1 = Tn + 1$ connections are required. The requirement would grow with the number of goroutines, so a spike of the traffic might lead to the dead lock if we set the limit for pool size. But if we don't not set the pool size limitation, it would harm the database performance as well.

### Takeaway
We should avoid concurrent connection in our handler. Only use concureent connections when there is a strong reason.
## How to avoid concurrent connections in a goroutine?
When I checked the DIOS code base, I found there are two kinds of scenario we would have concurrent connections in a goroutine.
1. Connection leakage
2. Using another connection with a uncommited transaction.
### Connection leakage
#### `QueryRow` without scan.
We use `QueryRow` instead of `Execute` to [update the database](https://github.com/deliveryhero/pd-dine-in-order-service/blob/14833b85b3d3a5d0ca923bbdc2a4b0275b56f375/tests/integration/cancel_customer_order_test.go#L251). We did not get the row and `Scan` it, which would leave a connection leakage, because `Scan` would close the connection internally[(ref)](https://cs.opensource.google/go/go/+/master:src/database/sql/sql.go;l=3335;bpv=1;bpt=0).

We can infer to other important critiria, like
1. Always call `Rows.Close()` to make sure no connection leakage
2. Close everything if you acquire a connection resource.
### Using another connection with a uncommited transaction.
#### Using another connection in a transaction
We start a transaction, but we did not use `tx.Query` but `conn.Query`. [here](https://github.com/deliveryhero/pd-dine-in-order-service/blob/14833b85b3d3a5d0ca923bbdc2a4b0275b56f375/internal/infra/database/stores/billing_transition_log.go#L68) and [here](https://github.com/deliveryhero/pd-dine-in-order-service/blob/master/tests/integration/base.go#L294). So there are two concurrent connections in a goroutine, one for the transaction and the other one for the query.

#### defet tx.Commit()
We start a transaction, and use another connection to query before the commit. [here](https://github.com/deliveryhero/pd-dine-in-order-service/blob/14833b85b3d3a5d0ca923bbdc2a4b0275b56f375/tests/integration/payment_captured_event_test.go#L310). The best practice would be use `MakeTransaction` function to ensure that we acquire a new connection only when the transaction has been committed.

## Summary
1. We should avoid concurrent connections in our handler.
2. The concurrent connections can happen when
* Connection leakage
* Using another connection with an uncommitted transaction.
3. For developers and reviewers, we should follow the following guideline
* Close everything after we acquire a connection
* For `QueryRow`, always get the result back and call `Scan`
* For `Query`, always call `Rows.Close()`
* Use `MakeTransaction`
* Don't use another connection inside a transaction.
* The concurrent connections in a goroutine should be only allowed when there's a strong reason.
## Action Item(To be discussed)
1. We should monitor the connection resource to avoid connection leakage. `sql` package provide [Stats](https://pkg.go.dev/database/sql#DB.Stats) function so we can get the pool information.
2. We should set up the connection limit when we ensure our codebase does not have connection leakage and concurrent connections in a goroutine.