[GODRIVER-579] nil context should always default to context.Background Created: 20/Sep/18  Updated: 14/Oct/22  Resolved: 14/Oct/22

Status: Closed
Project: Go Driver
Component/s: Internal
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: David Golden Assignee: Benji Rewis (Inactive)
Resolution: Won't Do Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

Some API functions allow a nil context, but this is inconsistently documented.

Others, like cursor.Close do not check for a nil context.

To assist users who don't want to explicitly specify context.Background() on all calls, all calls that take a context should check for a nil argument and replace it with context.Background().

All such functions should also have test coverage added for this case.



 Comments   
Comment by Benji Rewis (Inactive) [ 14/Oct/22 ]

We do not want to officially support passing nil contexts to any of our API. The official Go docs say:

Do not pass a nil Context, even if a function permits it. Pass context.TODO if you are unsure about which Context to use.

For the sake of posterity: our official policy is that users MUST pass non-nil contexts to any API. We do have some nil checks in the current version of the driver (cursor.Close() calls Close on the underlying batch cursor which does check for a nil context), and we cannot remove them as that may be backwards-breaking. We will not be adding more of these nil checks, though, as we do not want to encourage users to pass around nil contexts. Furthermore, even if we did add nil context checks to every user-facing function or method that takes a context, maintaining that pattern would be difficult. There is not a community-supported linter for that pattern (and we do not want to maintain one) and maintaining 100% test coverage for nil context checks is not feasible.

Comment by Jeffrey Yemin [ 01/Nov/18 ]

No objections. And no, the ticket doesn't get assigned to you.

Comment by Paul Fortin [ 31/Oct/18 ]

Also I do I get the ticket assigned to me?

Comment by Paul Fortin [ 31/Oct/18 ]

I would like to try to take this one on.  Any objections?  So far, in the code that I looked at all the changes are in the topology package (server.go, cursor.go and topology.go) - have I missed any your comments and directions are welcome.

I am going to do checks for ctx == nil and set it as required and unit test all the places I changed.

Comment by Kristofer Brandow (Inactive) [ 24/Sep/18 ]

While users should ideally never pass a nil context, since we already support this behavior it should be consistent.

Comment by David Golden [ 20/Sep/18 ]

client.Disconnect is another case that does not check, but should.

Generated at Thu Feb 08 08:34:31 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.