Add new Context parameter #352

MatthewDolan · 2017-08-31T04:51:56Z

Potential solution to #344

MatthewDolan · 2017-08-31T05:16:46Z

Couple of problems I just noticed with the build.

(1) The "context" package was added with Go 1.7 so adding it to the API would break compatibility with Go 1.6. Is that an issue? I guess I could copy it's definition into gorp, but that seems like it would break going forward.

(2) The Context API being added to db.Sql wasn't added until I think Go 1.8. I changed it so it will degrade gracefully to not passing on the context in earlier versions, but that will make the test I added fail unless I can exclude that test for earlier versions.

srenatus

Thanks for picking up that context issue!

Looks good to me -- but I, of course, can't help with the open questions. No idea what compatibility gorp wants to provide.

srenatus · 2017-08-31T06:35:38Z

gorp_test.go

+
+	ctx, cancel := context.WithCancel(context.Background())
+
+	defer time.AfterFunc(100 * time.Millisecond, cancel).Stop()


I suppose you did that on purpose, just wanted to point out that this is equivalent to:

ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) defer cancel()

srenatus · 2017-08-31T06:40:18Z

gorp_test.go

@@ -2360,6 +2361,31 @@ func TestPrepare(t *testing.T) {
 	}
 }

+func TestWithCanceledContext(t *testing.T) {


(2) The Context API being added to db.Sql wasn't added until I think Go 1.8. [...] but that will make the test I added fail unless I can exclude that test for earlier versions.

If you'd put that into its own file, you would be able to add // +build !go1.7, I believe? To make these tests not run on 1.7? (not entirely sure if it works like that, though)

nelsam · 2017-08-31T13:03:34Z

!go1.7 would make it only run on 1.6 and earlier. it should be +build go1.8. Other than that, yes - anything that requires 1.8 should be gated on build tags in separate files. I think it's somewhere in the readme that we aim to support the two most recent go versions. If I'm wrong about it being in the readme, well... I still think supporting the two most recent versions is plenty.

…

On Aug 31, 2017 00:42, "Stephan Renatus" ***@***.***> wrote: ***@***.**** approved this pull request. Thanks for picking up that context issue! Looks good to me -- but I, of course, can't help with the open questions. No idea what compatibility gorp wants to provide. ------------------------------ In gorp_test.go <#352 (comment)>: > @@ -2360,6 +2361,31 @@ func TestPrepare(t *testing.T) { } } +func TestWithCanceledContext(t *testing.T) { + if _, driver := dialectAndDriver(); driver != "mysql" { + t.Skip("Cancellation is not yet supported by all drivers. Known to be supported by mysql") + } + + dbmap := initDbMap() + defer dropAndClose(dbmap) + + ctx, cancel := context.WithCancel(context.Background()) + + defer time.AfterFunc(100 * time.Millisecond, cancel).Stop() I suppose you did that on purpose, just wanted to point out that this is equivalent to: ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)defer cancel() ------------------------------ In gorp_test.go <#352 (comment)>: > @@ -2360,6 +2361,31 @@ func TestPrepare(t *testing.T) { } } +func TestWithCanceledContext(t *testing.T) { (2) The Context API being added to db.Sql wasn't added until I think Go 1.8. [...] but that will make the test I added fail unless I can exclude that test for earlier versions. If you'd put that into its own file, you would be able to add // +build !go1.7, I believe? To make these tests not run on 1.7? (not entirely sure if it works like that, though) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#352 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-QBGNy7lnyVAm_GE5lh3n1RYmLx43rks5sdlXMgaJpZM4PITIB> .

MatthewDolan · 2017-08-31T14:08:43Z

Thanks for the comments @srenatus and @nelsam.

I removed 1.6 as a supported version for travis and added 1.8 and 1.9.

And I moved the cancelation test into a new file that only runs on 1.8 and above.

The tests are passing now for 1.7, 1.8, and 1.9.

bryanpaluch · 2017-08-31T14:46:11Z

Hi @MatthewDolan I think you need to add support in a few more places. QueryRow -> QueryRowContext, Query -> QueryContext, Prepare -> PrepareContext, and pretty much any other time there is an alternative Context function call with *sql.DB.

MatthewDolan · 2017-08-31T16:05:38Z

@bryanpaluch Oh you are totally right. Let me take another look.

MatthewDolan · 2017-08-31T18:17:07Z

@bryanpaluch Thanks. I updated the PR to add the other calls.

Build is passing for 1.7, 1.8, and 1.9.

nelsam · 2017-09-01T00:43:44Z

I'll review this weekend. I'm blocking out something on my schedule.

nelsam

Looks good for the most part, although I'd really like to see other database drivers tested. At least pq, since it seems like cancellation is tested there, and postgres is the database I use for all of my projects.

I'd say remove the check for mysql, see if it fails in CI, and only exclude drivers if they're failing in CI. If postgres is failing in CI, I'd like to dig in to why that is before excluding it from the test. When things don't work in postgres, it's usually not because postgres is wrong.

nelsam · 2017-09-04T02:00:05Z

gorp_go18_test.go

+)
+
+func TestWithCanceledContext(t *testing.T) {
+	if _, driver := dialectAndDriver(); driver != "mysql" {


I would really like to have a blacklist of drivers that don't support it, rather than a whitelist of drivers that do. If you're unsure, assume there is support - if it's not failing in travis, we'll deal with it when it becomes a problem.

Good call. It seems like mymysql is the only driver that has not added support (I filed it as an issue ziutek/mymysql#138).

Ran into one other problem though. Some sql dialects (namely sqlite) don't support native sleep calls. I checked the test for the sqlite driver and it looks like they have an attempt at testing a canceled context (https://github.com/mattn/go-sqlite3/blob/master/sqlite3_go18_test.go#L113), but I don't think it's as clean as sleep. I just excluded the languages that don't support sleep for now.

MatthewDolan · 2017-09-06T16:58:32Z

@nelsam Just in-case you missed my comment inline, I added an exclusion list instead of an inclusion list like you suggested.

Let me know if there is anything else I need to update.

Thanks for taking a look.

nelsam · 2017-09-06T17:07:58Z

Yep, I'll check on it soon. I've had a busy couple of days.

nelsam

Apologies for taking so long - I very seldom get a chance to review these things during the week, so I mostly have to wait for the weekend.

I'd rather not bump gorp's major version if we can help it. We can chat about other options, but putting a SleepDialect interface in the gorp_test package and using type assertions seems reasonable to me.

nelsam · 2017-09-09T16:07:32Z

gorp_go18_test.go

+func TestWithCanceledContext(t *testing.T) {
+	dialect, driver := dialectAndDriver()
+	if unsupportedDrivers[driver] {
+		t.Skip("Cancellation is not yet supported by all drivers. Not known to be unsupported in %s.", driver)


I think this should read Known to be unsupported or Not known to be supported, rather than Not known to be unsupported.

nelsam · 2017-09-09T16:14:03Z

dialect.go

@@ -49,6 +51,9 @@ type Dialect interface {
 	// string to truncate tables
 	TruncateClause() string

+	// string to sleep for d duration
+	SleepClause(d time.Duration) string


This is a breaking change and will force a v3 of gorp, since some gorp users may have implemented custom Dialect types. Can we instead add a new interface type, say SleepDialect, such that:

type SleepDialect interface { SleepClause(time.Duration) string }

And then remove the SleepClause methods on dialects that don't have a sleep clause?

In addition, since this method is only used by tests, can we move the SleepDialect interface to the test package? If it's in gorp's main package, then users expect that implementing SleepDialect gives them something from gorp, rather than gorp's tests.

nelsam · 2017-09-09T16:14:45Z

dialect_oracle.go

@@ -93,6 +94,10 @@ func (d OracleDialect) TruncateClause() string {
 	return "truncate"
 }

+func (d OracleDialect) SleepClause(s time.Duration) string {
+	return ""
+}


If we were to use a separate SleepDialect, you could just skip implementing this method.

nelsam · 2017-09-09T16:15:08Z

dialect_sqlite.go

@@ -88,6 +89,10 @@ func (d SqliteDialect) TruncateClause() string {
 	return "delete from"
 }

+func (d SqliteDialect) SleepClause(s time.Duration) string {
+	return ""
+}


Same here as with the OracleDialect - you wouldn't have to implement this method here.

nelsam · 2017-09-09T16:15:24Z

dialect_sqlserver.go

@@ -103,6 +104,10 @@ func (d SqlServerDialect) TruncateClause() string {
 	return "truncate table"
 }

+func (d SqlServerDialect) SleepClause(s time.Duration) string {
+	return ""
+}


Same as with Oracle and Sqlite

nelsam · 2017-09-09T16:16:20Z

gorp_go18_test.go

+		t.Skip("Cancellation is not yet supported by all drivers. Not known to be unsupported in %s.", driver)
+	}
+
+	if dialect.SleepClause(time.Second) == "" {


If we used a SleepDialect, this would change to:

sleeper, ok := dialect.(SleepDialect) if !ok {

nelsam

LGTM

nelsam · 2017-09-18T15:43:10Z

@MatthewDolan anything you want to add before merge? Otherwise, I can merge now.

MatthewDolan · 2017-09-18T15:48:09Z

Everything looks good to me. Feel free to merge.

Thanks for looking through this.

nelsam · 2017-09-18T15:51:20Z

Thanks for the submission! I'm trying to be more active with gorp, but you know how life and work go. Especially with gorp being something I no longer actively use.

This tries to introduce the ability to provide a context, while keeping the API surface intact, and not changing every single method signature. It is modelled slightly after how gorp had done the same thing three years ago: go-gorp/gorp#352 Signed-off-by: Stephan Renatus <[email protected]>

srenatus approved these changes Aug 31, 2017

View reviewed changes

nelsam mentioned this pull request Aug 31, 2017

Gorp new sql context support #344

Open

nelsam requested changes Sep 4, 2017

View reviewed changes

nelsam requested changes Sep 9, 2017

View reviewed changes

Add new Context parameter

fe96e85

nelsam approved these changes Sep 18, 2017

View reviewed changes

nelsam merged commit 659d466 into go-gorp:master Sep 18, 2017

MatthewDolan deleted the dolan-2017-08-30-add-context branch September 19, 2017 20:56

kf6nux mentioned this pull request Oct 2, 2017

Start using Contexts for database access letsencrypt/boulder#2819

Closed

srenatus mentioned this pull request Sep 22, 2020

management: add WithContext method go-auth0/auth0#142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Context parameter #352

Add new Context parameter #352

MatthewDolan commented Aug 31, 2017

MatthewDolan commented Aug 31, 2017

srenatus left a comment

srenatus Aug 31, 2017

srenatus Aug 31, 2017

nelsam commented Aug 31, 2017 via email

MatthewDolan commented Aug 31, 2017

bryanpaluch commented Aug 31, 2017

MatthewDolan commented Aug 31, 2017

MatthewDolan commented Aug 31, 2017

nelsam commented Sep 1, 2017

nelsam left a comment

nelsam Sep 4, 2017

MatthewDolan Sep 4, 2017

MatthewDolan commented Sep 6, 2017

nelsam commented Sep 6, 2017

nelsam left a comment

nelsam Sep 9, 2017

nelsam Sep 9, 2017

nelsam Sep 9, 2017

nelsam Sep 9, 2017

nelsam Sep 9, 2017

nelsam Sep 9, 2017

nelsam Sep 9, 2017 •

edited

Loading

nelsam left a comment

nelsam commented Sep 18, 2017

MatthewDolan commented Sep 18, 2017

nelsam commented Sep 18, 2017


		ctx, cancel := context.WithCancel(context.Background())

		defer time.AfterFunc(100 * time.Millisecond, cancel).Stop()

Add new Context parameter #352

Add new Context parameter #352

Conversation

MatthewDolan commented Aug 31, 2017

MatthewDolan commented Aug 31, 2017

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsam commented Aug 31, 2017 via email

MatthewDolan commented Aug 31, 2017

bryanpaluch commented Aug 31, 2017

MatthewDolan commented Aug 31, 2017

MatthewDolan commented Aug 31, 2017

nelsam commented Sep 1, 2017

nelsam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewDolan commented Sep 6, 2017

nelsam commented Sep 6, 2017

nelsam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsam Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

nelsam left a comment

Choose a reason for hiding this comment

nelsam commented Sep 18, 2017

MatthewDolan commented Sep 18, 2017

nelsam commented Sep 18, 2017

nelsam Sep 9, 2017 •

edited

Loading