Added debugging logic for our stopper pattern.

tiger5226 commented

2018-12-23 07:20:21 +01:00

(Migrated from github.com)

Added debugging logic for our stopper pattern. Useful for when a stopper doesn't stop right away as expected.

lyoshenka commented

2018-12-24 21:17:37 +01:00

(Migrated from github.com)

I'd prefer not to have code like this in the library. It adds extra complexity and it's not something most people want most of the time. For one thing, go already provides tools for debugging. For another, the code in this is so simple that you could copy/paste it locally and add debugging statements if you wanted to. Finally, I haven to seen any other go library do things like this.

tiger5226 commented

2018-12-25 05:08:12 +01:00

(Migrated from github.com)

I have had to use it a number of times now, and figured instead of rewriting it every time, I would just commit it this time. It is VERY useful when there is a lot of concurrency, like in chainquery.

When using the stopper pattern, you link the stopping of the application to a specific subset of go routines. Knowing which go routine, where, is not ending fast enough, makes it really easy to debug and find the root cause.

go already provides tools for debugging

Here the documentation referenced is clear how to get similar information. Niko and I did this once, but I can tell you this is MUCH easier and quicker than that method. There is no "figuring out" which routines are the applicable ones and no tracing the stack is required because the logging is functional.

adds extra complexity

I think this is a minimal impact and the user of the library can choose to do this based on their needs.

it's not something most people want most of the time

Minimal concurrency makes this added overhead, yes, but it is not required for the stopper to be used. The situations where you do not need this, can just use the standard methods. Although, I would argue it is still useful for when you get into a situation where a stopper doesn't stop right away.

this is so simple that you could copy/paste it locally and add debugging statements if you wanted to

Agreed, but I think if you find yourself re-writing the same thing over and over again, you might as well just finalize it. This being added to the library, makes it a 1 line change for me to turn on debugging of the stopper. Every time I close a routing it reports what else it is waiting on to finish closing.

I haven't seen any other go library do things like this

I would be interested in an equally beneficial way of doing something similar. The key items I am looking for:

A functional identification of dependant go routines ( ie the console statement that tells me exactly what this go routine is for ).
A simple way to turn it on without modifying dozens of lines of code.
When not enabled it has little to no impact on the application.

Different isn't always bad either. Domain specific debugging tactics can be priceless. This is just one specific to the stopper pattern. Not saying it can't be done better, but it works and fulfills it's expectations well.

I would argue that sometimes people think too generically. For example, look here. General debugging tactics. I get it, there are tools available. However, my need is very specific. delve is also another tool. This is overkill for me, but it is a generally acceptable and generic solution for debugging a golang app. I want to debug the stopper, quickly and easily.

If you still think this is too specific to my use case, I can remove this from the stopper library, just wanted to plead my case. When things are perfect, debugging is never needed, when there is a problem, you wish you had a switch like this to turn on.

I have had to use it a number of times now, and figured instead of rewriting it every time, I would just commit it this time. It is VERY useful when there is a lot of concurrency, like in chainquery. When using the stopper pattern, you link the stopping of the application to a specific subset of go routines. Knowing which go routine, where, is not ending fast enough, makes it really easy to debug and find the root cause. >go already provides tools for debugging [Here](https://stackoverflow.com/a/35290196) the documentation referenced is clear how to get similar information. Niko and I did this once, but I can tell you this is MUCH easier and quicker than that method. There is no "figuring out" which routines are the applicable ones and no tracing the stack is required because the logging is functional. >adds extra complexity I think this is a minimal impact and the user of the library can choose to do this based on their needs. >it's not something most people want most of the time Minimal concurrency makes this added overhead, yes, but it is not required for the stopper to be used. The situations where you do not need this, can just use the standard methods. Although, I would argue it is still useful for when you get into a situation where a stopper doesn't stop right away. >this is so simple that you could copy/paste it locally and add debugging statements if you wanted to Agreed, but I think if you find yourself re-writing the same thing over and over again, you might as well just finalize it. This being added to the library, makes it a 1 line change for me to turn on debugging of the stopper. Every time I close a routing it reports what else it is waiting on to finish closing. >I haven't seen any other go library do things like this I would be interested in an equally beneficial way of doing something similar. The key items I am looking for: - A functional identification of dependant go routines ( ie the console statement that tells me exactly what this go routine is for ). - A simple way to turn it on without modifying dozens of lines of code. - When not enabled it has little to no impact on the application. Different isn't always bad either. Domain specific debugging tactics can be priceless. This is just one specific to the stopper pattern. Not saying it can't be done better, but it works and fulfills it's expectations well. I would argue that sometimes people think too generically. For example, look [here](https://ttboj.wordpress.com/2016/02/15/debugging-golang-programs/). General debugging tactics. I get it, there are tools available. However, my need is very specific. [delve](https://github.com/derekparker/delve/blob/master/Documentation/usage/dlv_debug.md) is also another tool. This is overkill for me, but it is a generally acceptable and generic solution for debugging a golang app. I want to debug the stopper, quickly and easily. If you still think this is too specific to my use case, I can remove this from the stopper library, just wanted to plead my case. When things are perfect, debugging is never needed, when there is a problem, you wish you had a switch like this to turn on.

lyoshenka commented

2018-12-25 19:58:23 +01:00

(Migrated from github.com)

Can you show me an example of where/how this is used?

tiger5226 commented

2018-12-26 03:17:40 +01:00

(Migrated from github.com)

this is an example. In the most recent example, the stopper was taking up to a minute to stop the application. So I added this and it told me right away the problem was in the mempoolsync and it was the go routine the stopper was waiting for. There are 2 dozen go routines running if I were to dump the app. I only care about the ones required to end with a graceful shutdown. Know, which ones the stopper is waiting on specifically, is really helpful output. In this implementation, I just change a line of code to turn it on.

[this](https://github.com/lbryio/chainquery/blob/63b4e3cec10c0ee1db1c406b3734ea183a56a861/daemon/daemon.go) is an example. In the most recent example, the stopper was taking up to a minute to stop the application. So I added this and it told me right away the problem was in the mempoolsync and it was the go routine the stopper was waiting for. There are 2 dozen go routines running if I were to dump the app. I only care about the ones required to end with a graceful shutdown. Know, which ones the stopper is waiting on specifically, is really helpful output. In this implementation, I just change a line of code to turn it on.

lyoshenka commented

2018-12-28 13:16:42 +01:00

(Migrated from github.com)

i thought about it for a while but couldn't come up with a better approach that was also simple. i did have a few implementation suggestions and bugfixes: https://github.com/lbryio/lbry.go/pull/49

rename the functions to DebugAdd and DebugDone so its clear that they are meant for debugging
use mu instead of l for the mutex variable. its what ive seen used in the go standard libraries, and its technically more accurate (its a mutex, not a lock)
i suggest not makeing the map until DebugAdd is called. then it is not made in the typical non-debug case (and you don't have to call a separate constructor.
i didn't do this, but you should add comments to DebugAdd and DebugDone

i thought about it for a while but couldn't come up with a better approach that was also simple. i did have a few implementation suggestions and bugfixes: https://github.com/lbryio/lbry.go/pull/49 - rename the functions to DebugAdd and DebugDone so its clear that they are meant for debugging - use `mu` instead of `l` for the mutex variable. its what ive seen used in the go standard libraries, and its technically more accurate (its a mutex, not a lock) - i suggest not `make`ing the map until DebugAdd is called. then it is not made in the typical non-debug case (and you don't have to call a separate constructor. - i didn't do this, but you should add comments to DebugAdd and DebugDone

lyoshenka commented

2018-12-28 13:20:21 +01:00

(Migrated from github.com)

and now that ive thought about it some more, i think i misunderstood what you were trying to do with NewDebug. i see how it makes sense to leave AddNamed and DoneNamed in place, and simply turn on the debug behavior by changing the constructor. ignore that part of my suggested changes.

tiger5226 commented

2018-12-28 18:16:44 +01:00

(Migrated from github.com)

Ok I adjusted the other PR based on these comments.

Ok I adjusted [the other PR](https://github.com/lbryio/lbry.go/pull/49/commits/b0beb12fcfbbf9a778f13c6e4e6cb3d4033c5a0c) based on these comments.

Added debugging logic for our stopper pattern. #48