Skip to content
This repository has been archived by the owner on Feb 22, 2020. It is now read-only.

Show an error notification when misconfigured #79

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Show an error notification when misconfigured #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,29 @@
"description": "Submit code intel feedback",
"iconURL": "data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNCIgaGVpZ2h0PSIyNCIgdmlld0JveD0iMCAwIDI0IDI0Ij48cGF0aCBkPSJNMCAwaDI0djI0SDB6IiBmaWxsPSJub25lIi8+PHBhdGggZD0iTTExLjk5IDJDNi40NyAyIDIgNi40OCAyIDEyczQuNDcgMTAgOS45OSAxMEMxNy41MiAyMiAyMiAxNy41MiAyMiAxMlMxNy41MiAyIDExLjk5IDJ6TTEyIDIwYy00LjQyIDAtOC0zLjU4LTgtOHMzLjU4LTggOC04IDggMy41OCA4IDgtMy41OCA4LTggOHptMy41LTljLjgzIDAgMS41LS42NyAxLjUtMS41UzE2LjMzIDggMTUuNSA4IDE0IDguNjcgMTQgOS41cy42NyAxLjUgMS41IDEuNXptLTcgMGMuODMgMCAxLjUtLjY3IDEuNS0xLjVTOS4zMyA4IDguNSA4IDcgOC42NyA3IDkuNSA3LjY3IDExIDguNSAxMXptMy41IDYuNWMyLjMzIDAgNC4zMS0xLjQ2IDUuMTEtMy41SDYuODljLjggMi4wNCAyLjc4IDMuNSA1LjExIDMuNXoiLz48L3N2Zz4="
}
},
{
"id": "error",
"command": "open",
"title": "${get(context, `codeIntel.error.message`)}",
"commandArguments": [
"${get(context, `codeIntel.error.link`)}"
],
"actionItem": {
"description": "${get(context, `codeIntel.error.message`)}",
"iconURL": "data:image/svg+xml;charset=utf-8;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHZpZXdCb3g9IjAgMCAyNCAyNCIgaWQ9ImVsXzZNY29hYTdIbCI+PHN0eWxlPkAtd2Via2l0LWtleWZyYW1lcyBrZl9lbF9rSEpMSGNuRkJfX2FuXzYyTk9qRWRVanswJXtvcGFjaXR5OiAxO301MCV7b3BhY2l0eTogMC41O30xMDAle29wYWNpdHk6IDE7fX1Aa2V5ZnJhbWVzIGtmX2VsX2tISkxIY25GQl9fYW5fNjJOT2pFZFVqezAle29wYWNpdHk6IDE7fTUwJXtvcGFjaXR5OiAwLjU7fTEwMCV7b3BhY2l0eTogMTt9fSNlbF82TWNvYWE3SGwgKnstd2Via2l0LWFuaW1hdGlvbi1kdXJhdGlvbjogMnM7YW5pbWF0aW9uLWR1cmF0aW9uOiAyczstd2Via2l0LWFuaW1hdGlvbi1pdGVyYXRpb24tY291bnQ6IGluZmluaXRlO2FuaW1hdGlvbi1pdGVyYXRpb24tY291bnQ6IGluZmluaXRlOy13ZWJraXQtYW5pbWF0aW9uLXRpbWluZy1mdW5jdGlvbjogY3ViaWMtYmV6aWVyKDAsIDAsIDEsIDEpO2FuaW1hdGlvbi10aW1pbmctZnVuY3Rpb246IGN1YmljLWJlemllcigwLCAwLCAxLCAxKTt9I2VsX055VUdfdXBrRWZ7ZmlsbDogbm9uZTt9I2VsX2tISkxIY25GQl97ZmlsbDogcmVkOy13ZWJraXQtYW5pbWF0aW9uLWZpbGwtbW9kZTogYmFja3dhcmRzO2FuaW1hdGlvbi1maWxsLW1vZGU6IGJhY2t3YXJkcztvcGFjaXR5OiAxOy13ZWJraXQtYW5pbWF0aW9uLW5hbWU6IGtmX2VsX2tISkxIY25GQl9fYW5fNjJOT2pFZFVqO2FuaW1hdGlvbi1uYW1lOiBrZl9lbF9rSEpMSGNuRkJfX2FuXzYyTk9qRWRVajstd2Via2l0LWFuaW1hdGlvbi10aW1pbmctZnVuY3Rpb246IGN1YmljLWJlemllcigwLCAwLCAxLCAxKTthbmltYXRpb24tdGltaW5nLWZ1bmN0aW9uOiBjdWJpYy1iZXppZXIoMCwgMCwgMSwgMSk7fTwvc3R5bGU+PHBhdGggZD0iTTAgMGgyNHYyNEgweiIgaWQ9ImVsX055VUdfdXBrRWYiLz48cGF0aCBkPSJNMTIgMkM2LjQ4IDIgMiA2LjQ4IDIgMTJzNC40OCAxMCAxMCAxMCAxMC00LjQ4IDEwLTEwUzE3LjUyIDIgMTIgMnptMSAxNWgtMnYtNmgydjZ6bTAtOGgtMlY3aDJ2MnoiIGlkPSJlbF9rSEpMSGNuRkJfIi8+PC9zdmc+"
}
}
],
"menus": {
"editor/title": [
{
"action": "feedback",
"when": "showFeedback"
},
{
"action": "error",
"when": "showError"
}
],
"panel/toolbar": [
Expand Down
168 changes: 147 additions & 21 deletions src/lang-go.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
switchMap,
take,
finalize,
filter,
} from 'rxjs/operators'

import { ConsoleLogger, createWebSocketConnection } from '@sourcegraph/vscode-ws-jsonrpc'
Expand Down Expand Up @@ -189,31 +190,22 @@ async function tryToCreateAccessToken(): Promise<string | undefined> {
}

async function connectAndInitialize(
address: string,
address: URL,
root: URL,
token: string | undefined
): Promise<rpc.MessageConnection> {
const connectingToGoLangserverHelp = [
`Unable to connect to the Go language server at ${address}.`,
`Make sure ${'go.address' as keyof Settings} in your Sourcegraph settings is set to the address of the language server (e.g. wss://sourcegraph.example.com/go).`,
`Read the full documentation for more information: https://github.com/sourcegraph/sourcegraph-go`,
].join('\n')

const connectingToSourcegraphHelp = [
`The Go language server running on ${address} was unable to fetch repository contents from Sourcegraph running on ${sourcegraphURL()}.`,
`Make sure ${'go.sourcegraphUrl' as keyof Settings} in your settings is set to the address of Sourcegraph from the perspective of the language server (e.g. http://sourcegraph-frontend:30080 when running in Kubernetes).`,
`Read the full documentation for more information: https://github.com/sourcegraph/sourcegraph-go`,
].join('\n')

const connection = (await new Promise((resolve, reject) => {
const connection = (await new Promise<wsrpc.MessageConnection>((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use an async IIFE instead of the Promise constructor (Promise constructor should be avoided where possible, and it's possible here)

try {
const webSocket = new WebSocket(address)
const webSocket = new WebSocket(address.href)
const conn = createWebSocketConnection(wsrpc.toSocket(webSocket), new ConsoleLogger())
webSocket.addEventListener('open', () => resolve(conn))
webSocket.addEventListener('error', event => reject(new Error(connectingToGoLangserverHelp)))
webSocket.addEventListener('error', event => {
notifyUnableToConnectToLanguageServer(address)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant - this rejects the Promise, which is caught in the catch Block, where this function is also called. In general it's not good to have this logic in the event handler because if notifyUnableToConnectToLanguageServer throws itself for some reason that error will not be propagated and the Promise will be pending forever.

reject(event)
})
} catch (e) {
if ('message' in e && /Failed to construct/.test(e.message)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This err.message check (as any err.message check really) seems really brittle. What case is it trying to detect? Which case is it trying to not detect?

For example, I notice that it gets triggered in Chrome when you pass an http:// URL. Firefox however has a different Error message for that case. It also does not get triggered for an unsuccessful connection, or for passing an invalid URL.

console.error(connectingToGoLangserverHelp)
notifyUnableToConnectToLanguageServer(address)
}
reject(e)
}
Expand Down Expand Up @@ -246,8 +238,11 @@ async function connectAndInitialize(
}
)
} catch (e) {
if ('message' in e && (/no such host/.test(e.message) || /i\/o timeout/.test(e.message))) {
console.error(connectingToSourcegraphHelp)
if (
'message' in e &&
(/no such host/.test(e.message) || /i\/o timeout/.test(e.message) || /connection refused/.test(e.message))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do these errors come from? How can we be confident that these error messages are reliable? Are there no error codes/types, and if not, can we add them?

Which errors can happen that should not pass through this?

) {
notifyLanguageServerUnableToConnectToSourcegraph()
}
throw e
}
Expand Down Expand Up @@ -286,7 +281,7 @@ function repoNameFromDoc(doc: sourcegraph.TextDocument): string {
* Internally, this maintains a mapping from rootURI to the connection
* associated with that rootURI, so it supports multiple roots (untested).
*/
function mkSendRequest(address: string, token: string | undefined): Observable<SendRequest> {
function mkSendRequest(address: URL, token: string | undefined): Observable<SendRequest> {
const rootURIToConnection: { [rootURI: string]: Promise<rpc.MessageConnection> } = {}
async function connectionFor(root: URL): Promise<rpc.MessageConnection> {
if (rootURIToConnection[root.href]) {
Expand Down Expand Up @@ -563,6 +558,121 @@ function positionParams(doc: sourcegraph.TextDocument, pos: sourcegraph.Position
}
}

function notifyUnableToConnectToLanguageServer(address: URL): void {
sourcegraph.internal.updateContext({
showError: true,
'codeIntel.error.message': 'Error in language server setup. Click to open documentation.',
'codeIntel.error.link': 'https://sourcegraph.com/extensions/sourcegraph/go',
})
const portByProtocol: { [protocol: string]: string } = { 'wss:': '443', 'ws:': '80' }
sourcegraph.app.activeWindow!.showNotification(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's an error message, it should use NotificationType.Error

[
'**❌ Unable to access to the Go language server**',
'',
'Your browser could not access:',
'',
'```',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'```',
'```json',

`"go.serverUrl": "${address}"`,
'```',
'',
...(['ws:', 'wss:'].includes(address.protocol)
? [
'Troubleshoot using these commands:',
'',
`- **\`ping ${address.hostname}\`**`,
`- **\`telnet ${address.hostname} ${address.port || portByProtocol[address.protocol]}\`**`,
'',
]
: [
`The protocol **\`${
address.protocol
}\`** is invalid. Only **\`ws:\`** or **\`wss:\`** are valid.`,
'',
]),
'',
`To apply the fix:`,
'',
`- Correct the value in your [global settings](${
sourcegraph.internal.sourcegraphURL
}site-admin/global-settings).`,
'',
'To temporarily disable:',
'',
`- Comment out **\`go.serverUrl\`** in your [global settings](${
sourcegraph.internal.sourcegraphURL
}site-admin/global-settings).`,
].join('\n')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of markdown

)
}

function notifyLanguageServerUnableToConnectToSourcegraph(): void {
sourcegraph.internal.updateContext({
showError: true,
'codeIntel.error.message': 'Error in language server setup. Click to open documentation.',
'codeIntel.error.link': 'https://sourcegraph.com/extensions/sourcegraph/go',
})
sourcegraph.app.activeWindow!.showNotification(
[
'**❌ Error in Go language server setup**',
'',
`The Go language server was unable to fetch repository contents from Sourcegraph at:`,
'',
'```',
`"go.sourcegraphUrl": "${sourcegraphURL()}"`,
'```',
'',
'To troubleshoot this, **`docker exec`** into the go-langserver container and run:',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems tied to Docker. What if the user is using Kubernetes?

'',
`- **\`ping ${sourcegraphURL().hostname}\`**`,
`- **\`curl ${sourcegraphURL()}\`**`,
'',
`To apply the fix:`,
'',
`- Correct the value in your [global settings](${
sourcegraph.internal.sourcegraphURL
}site-admin/global-settings).`,
'',
'To temporarily disable:',
'',
`- Comment out **\`go.serverUrl\`** in your [global settings](${
sourcegraph.internal.sourcegraphURL
}site-admin/global-settings).`,
].join('\n')
)
}

async function canary(address: URL): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a doc comment? It's not obvious to me what this function's job is (maybe I just don't understand the name)

const data = await queryGraphQL('{currentUser{siteAdmin}}')
if (!data || !data.currentUser || data.currentUser.siteAdmin === undefined) {
console.log(
'Failed to determine whether or not the current user is an admin. Check the Network tab to see what went wrong.'
)
return
}
if (!data.currentUser.siteAdmin) {
// Don't show these notifications to normal users because they don't
// have access to global settings, and therefore can't fix this for all
// users.
return
}

try {
await new Promise<void>((resolve, reject) => {
let webSocket: WebSocket
try {
webSocket = new WebSocket(address.href)
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error would get caught by the outer catch, which has the same handling, so this try/catch appears redundant

notifyUnableToConnectToLanguageServer(address)
return
}
webSocket.addEventListener('open', () => resolve())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
webSocket.addEventListener('open', () => resolve())
webSocket.addEventListener('open', resolve)

webSocket.addEventListener('error', error => reject(error))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error is not an Error, it's an Event - always reject with Error instances

})
} catch (e) {
notifyUnableToConnectToLanguageServer(address)
}
}

/**
* Uses WebSockets to communicate with a language server.
*/
Expand All @@ -574,7 +684,23 @@ export async function activateUsingWebSockets(ctx: sourcegraph.ExtensionContext)
settings.next(sourcegraph.configuration.get<Settings>().value)
})
)
const langserverAddress = settings.pipe(map(settings => settings['go.serverUrl']))
const langserverAddress = settings.pipe(
map(settings => settings['go.serverUrl']),
concatMap(address => {
if (address === undefined) {
return [undefined]
}
try {
return [new URL(address)]
} catch (e) {
return []
}
})
)

ctx.subscriptions.add(
langserverAddress.pipe(filter((x: URL | undefined): x is URL => Boolean(x))).subscribe(url => canary(url))
)

const NO_ADDRESS_ERROR = `To get Go code intelligence, add "${'go.address' as keyof Settings}": "wss://example.com" to your settings.`

Expand Down