Add pprof flag for debugging

[sunshineplan]

This commit adds a pprof flag to the application, which allows enabling or disabling pprof for debugging purposes. This change is necessary to facilitate easier performance profiling during development and debugging sessions.

Related issue: #1806

[xiaokangwang]

_ "net/http/pprof" has too many side effects to be added to be added to the software by default. Would you mind to add to something that can be selectively compiled and are disabled/excluded from compilation by default.

[sunshineplan]

Actually, importing _ “net/http/pprof” here is not strictly necessary, as it is likely already included through other packages. I added it mainly for clarity and ease of reference. However, I can certainly try to add a build tag to make this feature selectively compiled and disabled by default.

[xiaokangwang]

Sadly with this change, pprof is now in conflict with other selective compile features. If there is another feature that are selected based on tag in this way, then it will basically need to do the same, on the same run.go files. Also there is too many duplicated codes making maintenance significantly more difficult .

To avoid that, make the least duplication by isolate the difference part into 2 files and duplicate the least amount of codes.

[sunshineplan]

Do you meaning that go build with multiple tags doesn't work as expected?

From Go 1.13 release notes:

The go build flag -tags now takes a comma-separated list of build tags, to allow for multiple tags in GOFLAGS. The space-separated form is deprecated but still recognized and will be maintained.

[database64128]

_ "net/http/pprof" has too many side effects to be added to be added to the software by default.

What side effects exactly? As far as I can see, the package init function just registers a bunch of routes at net/http.DefaultServeMux. I'm not sure how this would affect the operation of v2ray.

[xiaokangwang]

Do you meaning that go build with multiple tags doesn't work as expected?

From Go 1.13 release notes:

The go build flag -tags now takes a comma-separated list of build tags, to allow for multiple tags in GOFLAGS. The space-separated form is deprecated but still recognized and will be maintained.

What if someone else wish to add another function that requires changing the run.go, then there will need to be 4 version of run.go to work on every situation, then so on and so forth. With the current design of selective compile, there will be too many version of run.go require maintenance in no time.

[xiaokangwang]

_ "net/http/pprof" has too many side effects to be added to be added to the software by default.

What side effects exactly? As far as I can see, the package init function just registers a bunch of routes at net/http.DefaultServeMux. I'm not sure how this would affect the operation of v2ray.

Modify global state of shared component is not permitted. If every component do this, then there will be no way to ensure the safety and consistency of the program.

[database64128]

Modify global state of shared component is not permitted. If every component do this, then there will be no way to ensure the safety and consistency of the program.

Your point of not modifying the global state makes sense in many scenarios, but not here. In this case it's perfectly fine. V2ray itself does not make use of net/http.DefaultServeMux at all.

Blindly abiding by some arbitrary rule does not guarantee you "safety and consistency of the program". One could also say that spawning a goroutine modifies the global state of the Go runtime, which qualifies as a "shared component". Reading from a connection definitely modifies the global state too, since the net poller is also a "shared component".

[xiaokangwang]

Modify global state of shared component is not permitted. If every component do this, then there will be no way to ensure the safety and consistency of the program.

Your point of not modifying the global state makes sense in many scenarios, but not here. In this case it's perfectly fine. V2ray itself does not make use of net/http.DefaultServeMux at all.

Blindly abiding by some arbitrary rule does not guarantee you "safety and consistency of the program". One could also say that spawning a goroutine modifies the global state of the Go runtime, which qualifies as a "shared component". Reading from a connection definitely modifies the global state too, since the net poller is also a "shared component".

This rule is there to prevent some component expose sensitive information on these endpoint, and other component unintentionally share these information to others. Let's say we now have HTTP server without authentication that can be accessed locally, and without this rule, some other component can add a 'setpassword' endpoint to the default mux assuming only externally authenticated user has access to it. The same thing can also happen in reverse.

The principle here is that all components coexist with each other, include components that does not currently exist.

[sunshineplan]

What if someone else wish to add another function that requires changing the run.go, then there will need to be 4 version of run.go to work on every situation, then so on and so forth. With the current design of selective compile, there will be too many version of run.go require maintenance in no time.

I agree with your point that directly creating new versions of run.go could indeed lead to maintenance difficulties. We should consider introducing a concept similar to plugins, where the design allows for calling registered plugins at runtime. In this case, pprof could be implemented as a plugin.

Additionally, the primary reason for adding pprof was to enable users who encounter memory leaks to easily diagnose issues and provide relevant information. Therefore, this feature should be considered a basic functionality. Making it a selectively compiled feature is not a good idea, as it would require users to either compile the program themselves or download a version with pprof enabled, which clearly increases the difficulty for the users.

[xiaokangwang]

What if someone else wish to add another function that requires changing the run.go, then there will need to be 4 version of run.go to work on every situation, then so on and so forth. With the current design of selective compile, there will be too many version of run.go require maintenance in no time.

I agree with your point that directly creating new versions of run.go could indeed lead to maintenance difficulties. We should consider introducing a concept similar to plugins, where the design allows for calling registered plugins at runtime. In this case, pprof could be implemented as a plugin.

Additionally, the primary reason for adding pprof was to enable users who encounter memory leaks to easily diagnose issues and provide relevant information. Therefore, this feature should be considered a basic functionality. Making it a selectively compiled feature is not a good idea, as it would require users to either compile the program themselves or download a version with pprof enabled, which clearly increases the difficulty for the users.

Thanks for your revision. I think the init() from net/http/pprof still run in this case and I don't see a way to have it removed without selective compile.

I understand the ability to research the performance characteristic of program is an important feature, however, in most production environment it is never used. In order to use it, a development environment usually is already there, so requiring to compile it again isn't going to be a huge challenge.

Do you mind if I take over this merge request to add appropriate selective compile with tag and merge it?

[sunshineplan]

What if someone else wish to add another function that requires changing the run.go, then there will need to be 4 version of run.go to work on every situation, then so on and so forth. With the current design of selective compile, there will be too many version of run.go require maintenance in no time.

I agree with your point that directly creating new versions of run.go could indeed lead to maintenance difficulties. We should consider introducing a concept similar to plugins, where the design allows for calling registered plugins at runtime. In this case, pprof could be implemented as a plugin.
Additionally, the primary reason for adding pprof was to enable users who encounter memory leaks to easily diagnose issues and provide relevant information. Therefore, this feature should be considered a basic functionality. Making it a selectively compiled feature is not a good idea, as it would require users to either compile the program themselves or download a version with pprof enabled, which clearly increases the difficulty for the users.

Thanks for your revision. I think the init() from net/http/pprof still run in this case and I don't see a way to have it removed without selective compile.

I understand the ability to research the performance characteristic of program is an important feature, however, in most production environment it is never used. In order to use it, a development environment usually is already there, so requiring to compile it again isn't going to be a huge challenge.

Do you mind if I take over this merge request to add appropriate selective compile with tag and merge it?

You can take over the merge request.
However, I’d like to clarify my perspective for your reference. I made pprof a default component after considering real-world usage scenarios. The users of this software are not necessarily technical personnel. If a non-technical user encounters a memory leak, enabling the pprof feature via the command line might already be challenging without additional help. Asking such users to compile the software themselves would be exceedingly cumbersome, especially since they would first need to install the Go environment. Of course, memory leaks should be a rare issue for this software, and I only emphasize the importance of this feature because I’ve personally encountered it.

[xiaokangwang]

What if someone else wish to add another function that requires changing the run.go, then there will need to be 4 version of run.go to work on every situation, then so on and so forth. With the current design of selective compile, there will be too many version of run.go require maintenance in no time.

I agree with your point that directly creating new versions of run.go could indeed lead to maintenance difficulties. We should consider introducing a concept similar to plugins, where the design allows for calling registered plugins at runtime. In this case, pprof could be implemented as a plugin.
Additionally, the primary reason for adding pprof was to enable users who encounter memory leaks to easily diagnose issues and provide relevant information. Therefore, this feature should be considered a basic functionality. Making it a selectively compiled feature is not a good idea, as it would require users to either compile the program themselves or download a version with pprof enabled, which clearly increases the difficulty for the users.

Thanks for your revision. I think the init() from net/http/pprof still run in this case and I don't see a way to have it removed without selective compile.
I understand the ability to research the performance characteristic of program is an important feature, however, in most production environment it is never used. In order to use it, a development environment usually is already there, so requiring to compile it again isn't going to be a huge challenge.
Do you mind if I take over this merge request to add appropriate selective compile with tag and merge it?

You can take over the merge request. However, I’d like to clarify my perspective for your reference. I made pprof a default component after considering real-world usage scenarios. The users of this software are not necessarily technical personnel. If a non-technical user encounters a memory leak, enabling the pprof feature via the command line might already be challenging without additional help. Asking such users to compile the software themselves would be exceedingly cumbersome, especially since they would first need to install the Go environment. Of course, memory leaks should be a rare issue for this software, and I only emphasize the importance of this feature because I’ve personally encountered it.

I think here is the difference in mindset, that I believe non-tech users should only be expected to produce steps to reproduce the problem, but running profiling tool to actually find the root issue of the problem is the role of developers. Making profile tool available for non-development environments does not make sense as it does not enable non-tech users to actually find and fix problems.

The issue with profile that it contaminate globe state isn't your problem to begin with, it just make me uncomfortable to have it by default. In the future where this issue have been fixed: golang/go#42834 , it would be possible to have it by default.

[sunshineplan]

I think here is the difference in mindset, that I believe non-tech users should only be expected to produce steps to reproduce the problem, but running profiling tool to actually find the root issue of the problem is the role of developers. Making profile tool available for non-development environments does not make sense as it does not enable non-tech users to actually find and fix problems.

The issue with profile that it contaminate globe state isn't your problem to begin with, it just make me uncomfortable to have it by default. In the future where this issue have been fixed: golang/go#42834 , it would be possible to have it by default.

I understand your point, but please refer to the issue #3086 I submitted. The problem doesn’t generate any log errors, so it might be difficult to identify without handing it over to developers who have the necessary equipment to reproduce the issue. This imposes a hardware requirement on the developers. In this case, I actually believe that having users enable pprof and run it themselves would be a more convenient way.

Regarding another point, this is why I avoided using http.DefaultServeMux. However, as I mentioned earlier, even without my code, other dependencies have already imported the net/http/pprof package and contaminated the http.DefaultServeMux. So unless this is resolved by the Go team, there’s not much we can do, except perhaps not using http.DefaultServeMux.

[xiaokangwang]

I have checked the reason pprof get included in the current version of V2Ray by default:

depth -explain net/http/pprof .
. -> github.com/v2fly/v2ray-core/v5/main/distro/all -> github.com/v2fly/v2ray-core/v5/app/restfulapi -> github.com/go-chi/chi/v5/middleware -> net/http/pprof

Since restfulapi is nearly abandoned, it shouldn't be an issue if it get removed from default distribution.

[xiaokangwang]

I understand your point, but please refer to the issue #3086 I submitted. The problem doesn’t generate any log errors, so it might be difficult to identify without handing it over to developers who have the necessary equipment to reproduce the issue. This imposes a hardware requirement on the developers. In this case, I actually believe that having users enable pprof and run it themselves would be a more convenient way.

I do not understand why it is not possible to provide a custom build of the binary for user to test if a custom command line can be supplied by user. I understand it is harder, and create more fraction, but I believe a tool should not ship with these debug tools that could impact its security.

[sunshineplan]

I understand your point, but please refer to the issue #3086 I submitted. The problem doesn’t generate any log errors, so it might be difficult to identify without handing it over to developers who have the necessary equipment to reproduce the issue. This imposes a hardware requirement on the developers. In this case, I actually believe that having users enable pprof and run it themselves would be a more convenient way.

I do not understand why it is not possible to provide a custom build of the binary for user to test if a custom command line can be supplied by user. I understand it is harder, and create more fraction, but I believe a tool should not ship with these debug tools that could impact its security.

Maybe you’re right, I might have only considered convenience. If a custom binary were provided officially, the number of files would double, which is obviously unlikely. Moreover, the need to use pprof is extremely rare. In the end, I respect your decision.