Goroutine-local-storage implementation

[JT Olds]

Hey folks,

I know there's been a few discussions about this (and the relative impossibility thereof) here, but I just wanted to get some feedback on my company's goroutine-local-storage implementation.

https://github.com/SpaceMonkeyInc/gls

Anyway, our log lines are vastly improved, so it's a huge win for us.

-JT

[Jesse McNelis]

Wow, that's horrifying. 

--
=====================
http://jessta.id.au

[Dave Cheney]

You could make this abomination much simpler.

> --
> 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/groups/opt_out.

[Rémy Oudompheng]

The usual argument against goroutine-local storage is that goroutine
is essentially never the right level of granularity where you want to
attach a context.
I also wonder why your library, whose purpose is to provide
goroutine-local storage, also provides a helper "Go" whose purpose is
to propagate values across new goroutines.
Overall usage seems quite unpractical.

Rémy.

[james4k]

This was to improve your logs? Use a unique logger per relevant goroutine.

[Aram Hăvărneanu]

This is the most terrible thing I have seen in a very long time.

--
Aram Hăvărneanu

[Henrik Johansson]

Come on guys!

Most people have some legitimate need for something and I think calling things horrible is simply not the way to talk to people!

This is just horrible etiquette.

/ Henrik

[JT Olds]

You guys are all hilarious. This is pretty much exactly the response I
expected, lol

First off, I just want to point out that I agree that encoding values
on the stack is horrifying. But let me add some replies and
counterpoint :)

> The usual argument against goroutine-local storage is that goroutine
> is essentially never the right level of granularity where you want to
> attach a context.

This is the usual argument, and it's usually right. In fact, I would
personally discourage people from using goroutine-local storage in
nearly every case.

> This was to improve your logs? Use a unique logger per relevant goroutine.

Everything has tradeoffs, and I have the same problem as this guy:
https://groups.google.com/d/msg/golang-nuts/_Vv7Bzn8yH4/HRTU2ah_zM4J
AND the application codebase I'm working in is 88k lines of code and
counting, and I'm not exactly keen on changing every function that
logs anything to take a logging context, or belong to a logging
context, etc. In this case, goroutine-local storage is incredibly
useful. We can now associate what user request caused what log lines
in all areas of our codebase, without having to change *every*
interface.

Again, I'm only using this to improve my log lines. This is *similar*
to Popog's described use case here:
https://groups.google.com/d/msg/golang-nuts/Iyg3lKHV_lQ/DnJNqW9FPI8J

> I also wonder why your library, whose purpose is to provide
> goroutine-local storage, also provides a helper "Go" whose purpose is
> to propagate values across new goroutines.
> Overall usage seems quite unpractical.

Go is beautiful at parallelizing work, and in a few cases in our
codebase, we do a good deal of things in parallel, all in an attempt
to answer an HTTP request. In those cases, we have to kick off new
goroutines but want to preserve the HTTP context we've set up. The
"Go" method propagates our goroutine-local storage to these new
goroutines as we choose.

> You could make this abomination much simpler.

lol, Dave did actually reply directly with an actual suggestion after
this email, and he's right, you can actually get the goroutine id by
interacting with the runtime through C or ASM, but that seemed a
little less portable, and a little less future-proof as the language
changes (I doubt runtime.Callers is going anywhere). But yeah, maybe
that's worth doing to make things faster. I did think about it, as a
similar suggestion was made here:
https://groups.google.com/d/msg/golang-nuts/Iyg3lKHV_lQ/eDW8XX_zLCYJ

Here's a few more objections (and counterpoints):

* Holy crap, you're encoding values on the stack. You're ruining the
readability of the stack.

True, that's totally true. This is just one option in the debugging
tradeoff space.

* This is not very performant. Not only are you adding a bunch of
function calls but you have to call Callers to load a value.

Also completely true. I honestly have no idea what the performance
characteristics of runtime.Callers even is (it's just interacting with
debugging symbols I'm sure), but it doesn't matter to me. Often people
choose reduced performance for better debugging.

* I am horrified at how you are abusing a thing I never expected anyone to do.

Yeah, but check it out, we have really awesome logs now, without
changing tons of code and interfaces to take logging contexts, and
without rewriting any of my hundred libraries. I had a specific
problem that I was unable to find a better solution for. So, yay Go!

One final thought: from a PL perspective, thread or goroutine-local
storage is simply a way of attempting to give you lexically-bound but
dynamically scoped variables in a lexically scoped language. I
*completely* agree that dynamically scoped variables reduce the ease
at which one can reason about her program. But that (1) doesn't mean
they're totally devoid of use, and (2) lexical bindings to dynamically
scoped values address most every concern with dynamic variables. In
fact, in other languages, many things are essentially restricted
dynamically scoped variables, such as the current exception handler,
the current while/for loop to break out of, etc.

If you guys want me to add some kind of "DON'T USE THIS" warning to
the library, that's fine, and I *completely* both understand and
expected the "this is horrific and terrible" responses, but seriously,
this is so useful.

-JT

[JT Olds]

Due to the less-than-positive (but still hilarious) replies to the
library, just FYI I've moved the project to
https://github.com/jtolds/gls and added a quotes section. Evidently
Github will set up redirects from my employer's account.

[Steve McCoy]

This really is gross underneath, but the interface is pretty simple and I respect restrained abominations. ☺ Nice work.

[james4k]

The deed has been done, and it looks like you're not entertaining any other solutions, but consider that each http connection manages its own state, and that Go has these things called methods for acting on state in a nice, clean manner. What if you had a log method? Oh my goodness, this would mean that I can log messages using state from actual variables. I wouldn't have to encode my data in something silly like...the callstack! Man, this encapsulation thing is pretty cool.

[JT Olds]

On Tue, Nov 12, 2013 at 10:57 AM, james4k <ssl...@gmail.com> wrote:
> The deed has been done, and it looks like you're not entertaining any other
> solutions, but consider that each http connection manages its own state, and
> that Go has these things called methods for acting on state in a nice, clean
> manner. What if you had a log method? Oh my goodness, this would mean that I
> can log messages using state from actual variables. I wouldn't have to
> encode my data in something silly like...the callstack! Man, this
> encapsulation thing is pretty cool.

Of course I'm entertaining other solutions. I honestly haven't heard
one that really addresses my problem. I definitely considered a Log
method and in fact have a few. Yes, encapsulation is pretty cool.

[hstimer]

We were in this exact same situation a couple weeks ago. Highly parallel code base that needed contextual logging information -- i.e. transaction ID associated with each log entry. I had done this in the past with C++ TLS, and it worked great. For Go, the "very bad idea" did occur to me, but because we only had 5000 lines of source, we just brute forced it into the code base.

I would still like TLS, I don't have any other use case than contextual logging, so I'm completely open to other approaches that don't require adding a logging context variable to every function in the code base.

[Andrew Gerrand]

Wow, congrats. A while ago I tried to do this awful thing but got stuck. Your novel invention of the 16 special functions is very clever indeed!

Have you run any benchmarks? I imagine that this is very slow compared to the normal log package.

Andrew

[Kamil Kisiel]

Best project README I've seen in a while :)

Also, this scares me.

[roger peppe]

I have to say that the logging case is the only place that I really miss
having a goroutine id - when debugging many parallel threads of control,
it's actually really nice to be able to tease out the sequential pieces
from a log.

That said, using this to implement goroutine-local storage is IMHO
a really bad idea, and may well push you into a corner at some point.

The particular reason for this is that it precludes you from using
arbitrary concurrency, and it means that if you *do* happen
to call some (possibly external) function that uses concurrency,
and it calls a method in an interface you've defined, it could break.

As things are, Go has a wonderful flexibility about how one
might transform a program.

For example, if I have any code (*) of the form:

x := f()

I am free to transform it into:

var x T // where T is the type of the return of f
c := make(chan T)
go func() {
c <- f()
}()

This is incredibly useful - we can start functions concurrently;
we can send them to a central goroutine for execution;
we can time them out, etc.

But by making it possible for functions to depend on the actual
goroutine they're called in, you break that guarantee
and hence make everything a little more fragile and
less amenable to that kind of useful transformation,

The second possibility in particular (sending to another goroutine
means that even if all the functions in your ecosystem
use gls.Go instead of go, you *still* may not get the
correct context.

That's why using it for HTTP context is really not a great idea.
Much better to map from *http.Request to your context - then
anything you pass the request to has access to the context.

BTW it's no panacea for logging either - it's not uncommon to
have separate (and non-descendent) goroutines dealing
with some aspect of a request and it's nice if logging works
for those pieces too.

(*) panicking code excepted.

PS http://godoc.org/github.com/niemeyer/qml/tref

[JT Olds]

> Have you run any benchmarks? I imagine that this is very slow compared to the normal log package.

So, yeah, I wasn't incredibly concerned with performance, but I just
pushed some short benchmarks here:
https://github.com/jtolds/gls/commit/25ea7b721513 with the following
results:
BenchmarkGetValue 500000 2953 ns/op
BenchmarkSetValues 500000 4050 ns/op

Basically, calling GetValue (as a logging extension might do) adds a
few microseconds more of additional time on my aging i3 laptop. So
yeah, that will definitely add up.

> The particular reason for this is that it precludes you from using
> arbitrary concurrency, and it means that if you *do* happen
> to call some (possibly external) function that uses concurrency,
> and it calls a method in an interface you've defined, it could break.

By break, do you mean, you lose the context? Cause yes, that would
totally happen. This certainly doesn't solve everything (or anything,
lol).

> But by making it possible for functions to depend on the actual
> goroutine they're called in, you break that guarantee
> and hence make everything a little more fragile and
> less amenable to that kind of useful transformation,

Assuming someone actually uses this library for application logic,
yeah, that's too bad and a mistake, and I would probably discourage
anyone from doing so, cause it does increase program fragility.

Completely agreed that this is a horrible, tragic idea to use as a
general case solution for many problems.

As it stands though, the absolute worst case here for my specific case
is my log lines lose some information. I'm okay with that. I'm happy
to continue to let people refactor things as they want. If for some
reason there's a specific log line in a concurrent section of code
that we really need context on, we can handle that specially, but
there's no reason to be heavy handed about it.

> BTW it's no panacea for logging either - it's not uncommon to
> have separate (and non-descendent) goroutines dealing
> with some aspect of a request and it's nice if logging works
> for those pieces too.

totally. Dustin Sallings also pointed that out in #go-nuts yesterday,
and it's a great point.

You said it best, it's no panacea. It's just another (crazy, horrific)
tool in the toolbox. In no way does this add context to 100% of all
log lines current and future. But it does add context to 98% of my
current log lines, which is pretty neat.

If we need to refactor more things into worker/queue models or who
knows what else down the road; that's totally fine. Then we're left
with the existing suggestion of passing along a logging context in
with all the other program arguments, and as many have pointed out
here, that's preferable usually anyway.

-JT

[JT Olds]

> PS http://godoc.org/github.com/niemeyer/qml/tref

Whoops, I missed this. I'm not too familiar with Go runtime internals,
but as far as I can tell from cgocall.c, "m" is the current thread? So
I would need to lock the current goroutine to the current thread to
use "m"? Wouldn't I rather use G or something?

Anyway, yeah, I bet I could make this a *lot* faster by using runtime
internals. Even just some stack-specific context like frame pointers
or something would be huge.

On Wed, Nov 13, 2013 at 2:05 AM, roger peppe <rogp...@gmail.com> wrote:

[roger peppe]

On 13 November 2013 14:40, JT Olds <jto...@xnet5.com> wrote:
> Assuming someone actually uses this library for application logic,
> yeah, that's too bad and a mistake, and I would probably discourage
> anyone from doing so, cause it does increase program fragility.

Ah, I assumed that when you said "we have to kick off new
goroutines but want to preserve the HTTP context we've set up",
that was talking about application logic, not just logging.

[JT Olds]

Yeah, it's a good point. I should probably add a warnings section to
the readme lest some poor soul find my library and use it poorly.

Here's yet another problem with the library: currently, it reads its
tag values off the stack by asking for up to the last 64 callers. If
the stack grows larger than 64 entries, it will fail to read the tag
values off and you lose the context.

Basically, anything that uses this (if at all) should expect things
stored in the ContextManager object to be ephemeral at best.

-JT

[Ingo Oeser]

What about adding the logger to the struct providing the context (e.g. the transaction, the request, the connection, etc)?

Then you can do transaction.logger.Info("foobar")

If you embed it, then you can even do: transaction.Info("foobar")

[JT Olds]

That totally 100% works. That said, my HTTP endpoints are just simple
gateways into all sorts of various clients and library calls in an
85kloc codebase, and refactoring all of them in this way is a little
bit on the disappointing side. :)

[hstimer]

I think the least bad answer is using the go routine internal ID. I think that idea came from Ian.

Options:

1) The abomination - hunting up the stack

2) The ugly - forcing a context value through all your routines

3) The unportable - using the go routine internal ID

When I get around to it, I'm going to re-implement our #2 code, using #3. I would rather be unportable than ugly.

[roger peppe]

On 15 November 2013 04:01, hstimer <hans....@gmail.com> wrote:
> I think the least bad answer is using the go routine internal ID. I think
> that idea came from Ian.
>
> Options:
> 1) The abomination - hunting up the stack
> 2) The ugly - forcing a context value through all your routines
> 3) The unportable - using the go routine internal ID
>
> When I get around to it, I'm going to re-implement our #2 code, using #3. I
> would rather be unportable than ugly.

Please don't. #2 is not ugly - it's explicit, which is a different thing,
and not a bad thing. And if you do that, then you will never
be able to use your code in contexts where unsafe is not an
option (e.g. google appengine).

[Jens Alfke]

On Tuesday, November 12, 2013 9:57:52 AM UTC-8, james4k wrote:

consider that each http connection manages its own state, and that Go has these things called methods for acting on state in a nice, clean manner. What if you had a log method? Oh my goodness, this would mean that I can log messages using state from actual variables. I wouldn't have to encode my data in something silly like...the callstack! Man, this encapsulation thing is pretty cool.

Yeah, cheap sarcasm, and talking to people like children, is so much more fun than actually reading the substance of the discussion, isn't it?

You clearly missed the part where he said:

the application codebase I'm working in is 88k lines of code and 
counting, and I'm not exactly keen on changing every function that 
logs anything to take a logging context, or belong to a logging 
context, etc.

I understand the issue, having a somewhat similar situation. There are functions that log that are in separate modules several layers below the HTTP handler level, and it would be a lot of work to plumb some kind of unified context all the way down to them.

I just gave up on having clean logs, rather than going to the lengths that JT did, but I'm not going to be an asshat on a pulpit and shame him for doing it.

--Jens

[eas...@gmail.com]

There's another implementation:

https://github.com/easeway/gls

which looks better and cleaner...

[Florin Patan]

With the risk of spamming all the others in this thread, sorry all (btw, please don't resurrect old discussions, start new ones)....

Why would one need such a functionality? Rather than writing:

gls.With(context, myWork)

why not simply change "myWork" to accept the context parameter?

I understand that magic is nice, it suddenly gives one the ability to bypass some refactoring work and get something done quickly, but at what price?

Isn't a programmer's job to write code, not magic?

Isn't there enough software out there written in Go, that is waaaay more advanced than a simple hello world, and dare I say it whatever use-case you had to solve, where people don't need tls in order for their app to work as expected?

And to prove my point about magic, what happens if suddenly one wants to transform a call to "myWork()" into a separate goroutine, "go myWork()"? With passing a context parameter things will still work as expected, with this one has to call the whole thing differently (more magic, yaaay). But the real deal breaker would be if someone in the middle (new programmer, new version of a lib, whatever) launches a goroutine without using the functionality provided, then it all goes away. Also this way the compiler has no chance to prevent the mistake at compile time so one will catch this at test / runtime.

It might be that it's just my grumpy pov right now and I know nothing so please take this comment with a grain of salt.

Cheers

[eas...@gmail.com]

I agree most of your point, whenever refactoring is possible.

Sometimes when we are using a go library which put a thick call stack between your application and your handler, and usually it's impossible for you to refactor a third-party middleware. It will be a little ugly using a wrapper function to bind a context for every handler e.g. setHandler(func(args...) { myHandler(ctx, args...) }). Sometimes the situation is more complicated, and usually end-up we have to put context as the first arg in all functions in the application.