Skip to content

fix: defer context to prevent goroutine leak - #7847

Open
System-Glitch wants to merge 1 commit into
go-gorm:masterfrom
System-Glitch:fix/context-leak
Open

System-Glitch wants to merge 1 commit into
go-gorm:masterfrom
System-Glitch:fix/context-leak

Conversation

@System-Glitch

@System-Glitch System-Glitch commented Aug 28, 2026 •

Copy link
Copy Markdown
  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

  • Fix context leak by deferring context cancel if DefaultContextTimeout is set in the config.
  • The statement is cloned with the new context for each query. This way transactions are not broken because the context was canceled after BEGIN returned. Each query will have its own timeout context and statements can be re-used safely.
  • To make this work without breaking db.Scan(), I moved the rows scanning into a new callback "gorm:scan_rows".
    • This was necessary because the Row callbacks return before calling rows.Next() in db.Scan(), leading to a context canceled error.
    • This allows the rows reading/scanning to be included in the timeout duration, which makes sense to me.
    • This doesn't break db.Row() and db.Rows() because the new callback doesn't activate in those cases.
    • The DefaultContextTimeout option does nothing on db.Row() and db.Rows(). Developers need to manage the context and timeout themselves since they are scanning manually.

User Case Description

DefaultContextTimeout setting is great but causes context leaks. Fixing this issue makes the feature a very useful one.

Related to:

@System-Glitch
System-Glitch marked this pull request as draft August 31, 2026 09:24
@System-Glitch
System-Glitch marked this pull request as ready for review August 31, 2026 09:29
@System-Glitch
System-Glitch force-pushed the fix/context-leak branch 2 times, most recently from 50cae17 to 832eb27 Compare August 31, 2026 09:46
@System-Glitch

Copy link
Copy Markdown
Author

I have a unit test suite for this, based on go-sqlmock and testing/synctest. However there aren't any test of this kind in the project yet and that would add a dependency in the go.mod. I can add the test if you want, just say the word!

@System-Glitch

Copy link
Copy Markdown
Author

Rebased and fixed lint issue.

This branch has not been deployed

No deployments
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.

1 participant