Daniel, I would characterize the use case you describe as a very indirect way of setting these options on transactions. That's can be a good thing when contexts are already passed around, and coming up with new ways of passing such options down through a few layers of code is some amount of work. It's certainly convenient that they are part of the context in that case.
That said, I believe there are large downsides to this indirect API design. Even when you benefit from this convenience, it encourages a form of "spooky action at a distance" programming where it's not clear at the BeginContext call site what the properties of the transaction will be. Using Go has made me very appreciative of other language features that directly remove this "spooky action at a distance" from other programming languages. Not having exceptions is a great example of this. I would say that Go trades convenience in favor of clarity in those circumstances, and I'm concerned that we're making the opposite tradeoff here. One can then easily argue that nothing is stopping me or anybody else from forgoing the indirect nature and finding a way of propagating these options down to the code that actually calls BeginContext. In fact, that's probably what I would do. The problem then is that the API is needlessly awkward; it would be much more straightforward to expose an API with functional options if that's the encouraged way of using it. Another worry of mine is that contexts are passed on to other functions. Even though it's rare that a single request consists of multiple transactions, it still happens and I wouldn't be too surprised to find people accidentally setting the wrong transaction properties simply because it was accidentally inherited from the context that was used to begin another transaction. Again, being more explicit in the API design would cause people to stop and think before passing along a set of transaction options from an unrelated transaction, similar to how Go's explicit error handling forces you to think about errors, even if it's less convenient. To end with a concrete proposal, I think changing the signature to be "func (*DB) BeginContext(ctx context.Context, opts ...TxOption)" or similar (and then changing the transaction properties to be TxOptions) would solve most of the issues I highlighted above. André On Friday, December 9, 2016 at 9:14:08 PM UTC+1, Daniel Theophanes wrote: > > Hi Chandra, > > In my view using context to store value for these uses (Read-Only and > Isolation Level) is somewhat of a gray area. They are not a classical > request only scope, like a tracing ID, but they also aren't strictly > limited to a single request either. Say you wanted to kick off a request, > but ensure you don't modify the database in anyway. you could pass in a > context with ReadOnly set and any further request function will follow that > directive. Similarly, by allowing you to set the Isolation at a higher > level, you can effectively influence the behavior of the queries for any > subsequent request action. > > Should we do it this way? Maybe, maybe not. I think there are benefits to > this approach, and I think they can be considered request scoped, even if > in the degenerate case they are set at the query level. However, I think it > will be really common to set these when dispatching requests in the router > and using them for the subsequent requests. > > Has /report/ prefix? Set to ReadOnly and Isolation X > Has /api/ prefix? Set Isolation to Y > > At least that is how I would envision using them. I have been known to be > wrong before. > What do you think? -Daniel > > > On Thursday, December 8, 2016 at 11:27:36 PM UTC-8, Chandra Sekar S wrote: >> >> Bump >> >> -- >> Chandra Sekar.S >> >> On Wed, Dec 7, 2016 at 10:24 AM, Chandru <chand...@gmail.com> wrote: >> >>> I can understand db/sql using Context for cancellation. It is the >>> optional arguments to BeginContext like IsolationLevel and read-only flag, >>> which are not request-specific, that seem to contradict context's >>> documentation. >>> >>> -- >>> Chandra Sekar.S >>> >>> On Tue, Dec 6, 2016 at 9:50 PM, <parais...@gmail.com> wrote: >>> >>>> Either the doc should be changed or the std lib should follow the >>>> spirit of the doc. But my understanding is that context.Context is not >>>> just >>>> about HTTP request/response cycle but deals with anything that needs >>>> cancellation or a resource clean up signal , some kind of DIY RAII . In >>>> any >>>> case the documentation should be clarified. >>>> >>>> Le mardi 6 décembre 2016 16:48:48 UTC+1, Chandra Sekar S a écrit : >>>>> >>>>> Documentation of the context package says, >>>>> >>>>> "Use context Values only for request-scoped data that transits >>>>> processes and APIs, not for passing optional parameters to functions." >>>>> >>>>> sql.BeginContext introduced in 1.8, uses Context to receive options >>>>> like IsolationLevel and read-only flag. These are neither >>>>> request-specific >>>>> nor cross-cutting. They are options that are typically specific to a type >>>>> of operation, but common to all requests. >>>>> >>>>> Isn't this use in db/sql contradicting the recommendation in context's >>>>> doc? >>>>> >>>>> -- >>>>> Chandra Sekar.S >>>>> >>>> -- >>>> You received this message because you are subscribed to the Google >>>> Groups "golang-nuts" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to golang-nuts...@googlegroups.com. >>>> For more options, visit https://groups.google.com/d/optout. >>>> >>> >>> >> -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
