Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: handle 404-ing links in documentLink #32994

Closed
stamblerre opened this issue Jul 8, 2019 · 4 comments
Closed

x/tools/gopls: handle 404-ing links in documentLink #32994

stamblerre opened this issue Jul 8, 2019 · 4 comments

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jul 8, 2019

Related to discussion on microsoft/vscode-go#2614.
Would it be reasonable to test that a link is valid in textDocument/documentLink?

@gopherbot gopherbot added this to the Unreleased milestone Jul 8, 2019
@litleleprikon

This comment has been minimized.

Copy link

@litleleprikon litleleprikon commented Jul 10, 2019

HI @stamblerre! I can work on this issue especially as I am working now on #32339. The question is: is it really necessary to send HTTP request for each link in the file? I can see at least two possible issues here:

At first right now with implemented #32339 the documentLinks sometimes takes seconds to return the list of links. This is request for file with about 900 lines of code: [Trace - 4:46:35 PM] Received response 'textDocument/documentLink - (3)' in 3663ms.. Making the HTTP request for each link will increase this time dramatically.

The second issue is that I am not sure that developers expect network usage from language server especially during working with mobile internet. Making a bunch of HTTP calls on each documentLinks request can cause not only clogging of internet connection but also increased bills if you are working on mobile internet. I am not sure that scenario of usage mobile internet is wide and probably needs discussion.

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Jul 10, 2019

Yeah, I am not sure that we should be doing this either. We could do some caching for links we've already seen, but I'm not sure that it's worth it. I filed this issue mostly to start a discussion, rather than to necessarily propose a solution.

@DisposaBoy

This comment has been minimized.

Copy link

@DisposaBoy DisposaBoy commented Jul 11, 2019

[...] HTTP request [...]

FWIW, I think this is a bad idea. This is what happened when iTerm2 did something similar: https://nvd.nist.gov/vuln/detail/CVE-2015-9231.
EDIT: relevant discussion: https://news.ycombinator.com/item?id=15286956

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Jul 11, 2019

Thanks for sharing. I think this issue is probably not very pressing, and I just wanted to open it to note that this has been mentioned, but I think we can close it for now. I'm certain this will come up on the future, and we can figure out a way to handle it then.

@stamblerre stamblerre closed this Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.