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.

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

Implement [Database Template] [MySQL] [PostgreSQL] [SQLite] Close Database Connection #204

Merged
merged 5 commits into from
May 13, 2024

Conversation

H0llyW00dzZ
Copy link
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

Problem/Feature

By implementing this Close Database Connection, it can be useful in scenarios such as graceful shutdown, for example, in Kubernetes (k8s).

Description of Changes:

  • [+] feat(mysql.tmpl): add Close() method to Service interface and its implementation
  • [+] docs(mysql.tmpl): add comments for Service interface and its methods

Note: By using the "log.Printf" method, it is more concise and idiomatic Go.

Checklist

  • I have self-reviewed the changes being requested
  • I have updated the documentation (if applicable)

Comment on lines +72 to +119
func (s *service) Close() error {
log.Printf("Disconnected from database: %s", dbname)
return s.db.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't we want to try to close before logging? What happens if s.db.Close returns an error? Will it still have disconnected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't we want to try to close before logging? What happens if s.db.Close returns an error? Will it still have disconnected?

The log.Printf("Disconnected from database: %s", dbname) statement is useful when used with goroutines, such as for asynchronously starting and shutting down the server. However, it's recommended to handle the error returned by s.db.Close before logging the disconnection message. This way, you can ensure that the database connection is properly closed, even if an error occurs during the process. If s.db.Close returns an error, it doesn't necessarily mean that the connection has been successfully disconnected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@briancbarrow example how it work its like this

$ go run cmd/api/main.go
2024/04/04 20:52:02 [H0llyW00dzZ Firewall] [INFO] Connected to database: defaultdb
2024/04/04 20:52:04 [H0llyW00dzZ] [INFO] Starting server on :8080

 ┌───────────────────────────────────────────────────┐ 
 │                    H0llyW00dzZ                    │ 
 │                   Fiber v2.52.4                   │ 
 │               http://127.0.0.1:8080               │ 
 │       (bound on host 0.0.0.0 and port 8080)       │ 
 │                                                   │ 
 │ Handlers ............ 537 Processes ........... 1 │ 
 │ Prefork ....... Disabled  PID ............. 15836 │ 
 └───────────────────────────────────────────────────┘ 

2024/04/04 20:52:12 [H0llyW00dzZ] [INFO] Shutting down server... reason: interrupt
2024/04/04 20:52:12 [H0llyW00dzZ] [INFO] Disconnected from database: defaultdb
2024/04/04 20:52:12 [H0llyW00dzZ] [INFO] Database connection closed.
2024/04/04 20:52:17 [H0llyW00dzZ] [INFO] Server exiting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That My Fiber application is fully managed by goroutines running in parallel.

@briancbarrow
Copy link
Collaborator

Hey @H0llyW00dzZ, another good idea but we'd need the changes in all the db files, not just MySQL.

@H0llyW00dzZ
Copy link
Contributor Author

Hey @H0llyW00dzZ, another good idea but we'd need the changes in all the db files, not just MySQL.

@briancbarrow, have to test later one by one for other database

@briancbarrow
Copy link
Collaborator

Hey @H0llyW00dzZ, another good idea but we'd need the changes in all the db files, not just MySQL.

@briancbarrow, have to test later one by one for other database

I'm going to move this PR into draft until the other databases have been supported. Ping me again when it is ready.

@briancbarrow briancbarrow marked this pull request as draft April 6, 2024 22:44
@H0llyW00dzZ
Copy link
Contributor Author

Hey @H0llyW00dzZ, another good idea but we'd need the changes in all the db files, not just MySQL.

@briancbarrow, have to test later one by one for other database

I'm going to move this PR into draft until the other databases have been supported. Ping me again when it is ready.

I think any database driver supported database/sql it supported to call this Close()

@briancbarrow
Copy link
Collaborator

Hey @H0llyW00dzZ, another good idea but we'd need the changes in all the db files, not just MySQL.

@briancbarrow, have to test later one by one for other database

I'm going to move this PR into draft until the other databases have been supported. Ping me again when it is ready.

I think any database driver supported database/sql it supported to call this Close()

Have you tested this that it works with the other databases?

@H0llyW00dzZ
Copy link
Contributor Author

Hey @H0llyW00dzZ, another good idea but we'd need the changes in all the db files, not just MySQL.

@briancbarrow, have to test later one by one for other database

I'm going to move this PR into draft until the other databases have been supported. Ping me again when it is ready.

I think any database driver supported database/sql it supported to call this Close()

Have you tested this that it works with the other databases?

currently only Redis and MySQL, was implemented this as well for Redis and MySQL

image

- [+] feat(mysql.tmpl): add Close() method to Service interface and its implementation
- [+] docs(mysql.tmpl): add comments for Service interface and its methods

Note: By using the "log.Printf" method, it is more concise and idiomatic Go.
@H0llyW00dzZ
Copy link
Contributor Author

H0llyW00dzZ commented Apr 18, 2024

@briancbarrow Anyway, it's possible to close the connection for any database supported by the standard library's . So, I think I'm going to implement this for MySQL, PostgreSQL, and SQLite.

@H0llyW00dzZ H0llyW00dzZ changed the title Implement [Database Template] [MYSQL] Close Database Connection Implement [Database Template] [MySQL] [PostgreSQL] [SQLite] Close Database Connection Apr 18, 2024
- [+] feat(postgres.tmpl): add Close method to Service interface and its implementation
- [+] feat(sqlite.tmpl): add Close() method to Service interface and its implementation in service struct
- [+] fix(dbdriver): update log message to use correct database name and URL variables in postgres and sqlite templates
- [+] fix(postgres.tmpl): change DB_DATABASE to database in disconnect log message
@H0llyW00dzZ H0llyW00dzZ marked this pull request as ready for review April 18, 2024 15:41
@H0llyW00dzZ
Copy link
Contributor Author

@briancbarrow Done, by the way. Only Redis and MongoDB are left, and they must be implemented independently because they are not supported by the standard library's.

@H0llyW00dzZ
Copy link
Contributor Author

Note

Also note that this is pretty useful when used for restarting or shutting down, because if the server or service is shut down or restarted without closing the connection, the connection will remain open and could lead to potential deadlocks/hangs for the database (e.g., MySQL, PostgreSQL, SQLite).

@briancbarrow briancbarrow merged commit bfbac5a into Melkeydev:main May 13, 2024
127 checks passed
@H0llyW00dzZ H0llyW00dzZ deleted the mysql-connection branch May 13, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants