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

fix: update context handling in *Context db methods to stop context pollution #48

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

jsteenb2
Copy link
Collaborator

@jsteenb2 jsteenb2 commented Feb 14, 2023

this is a fix for QueryContext, ExecContext context pollution. It also addresses PrepareStmt as that is a different lifecycle. Gorm makes heavy use of PrepareStmt. With this we have the ability to issue individual contexts for a {Query|Exec|Prepare}Context and cancel them. AKin to the following:

func doThingWithin3Seconds(ctx context.Context, db *sql.DB) {
     ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
     // cancels regardless of how the db method below results
     // should not affect future queries to the db if its successful though
     defer cancel() 
    
    // ... trim

    rows, err := db.QueryContext(....) // or ExecContext
    // ... trim
}

this fixes it in our old legacy gorm tests and our newer pgx|slqx tests.

issue: #43

@@ -103,6 +103,9 @@ type conn struct {
drv *txDriver
saves uint
savePoint SavePoint

cancel func()
ctx interface{ Done() <-chan struct{} }
Copy link
Collaborator Author

@jsteenb2 jsteenb2 Feb 14, 2023

Choose a reason for hiding this comment

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

inlining this interface to avoid calling into context.Context, which would break backwards compatability with versions of go that predate the context pkg

@jsteenb2 jsteenb2 force-pushed the fix/context_pollution branch 2 times, most recently from dfd4c35 to adb6c51 Compare February 14, 2023 21:56
Copy link
Member

@l3pp4rd l3pp4rd left a comment

Choose a reason for hiding this comment

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

thanks, sorry I saw your email, but forgot to have a look.

@l3pp4rd l3pp4rd merged commit 164f0b8 into DATA-DOG:master Feb 23, 2023
@jsteenb2 jsteenb2 deleted the fix/context_pollution branch February 23, 2023 13: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.

None yet

2 participants