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

Fixes for Language Server sync server #8514

Merged
merged 17 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ app/ide-desktop/lib/dashboard/playwright-report/
app/ide-desktop/lib/dashboard/playwright/.cache/
app/gui/view/documentation/assets/stylesheet.css
app/gui2/rust-ffi/pkg
app/gui2/src/assets/font-*.css
Cargo.lock
build.json
app/gui2/playwright-report/
Expand Down
54 changes: 54 additions & 0 deletions app/gui2/shared/__tests__/yjsModel.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { rangeEncloses, rangeIntersects, type ContentRange } from 'shared/yjsModel'
import { expect, test } from 'vitest'

type RangeTest = { a: ContentRange; b: ContentRange }

const equalRanges: RangeTest[] = [
{ a: [0, 0], b: [0, 0] },
{ a: [0, 1], b: [0, 1] },
{ a: [-5, 5], b: [-5, 5] },
]

const totalOverlap: RangeTest[] = [
{ a: [0, 1], b: [0, 0] },
{ a: [0, 2], b: [2, 2] },
{ a: [-1, 1], b: [1, 1] },
{ a: [0, 2], b: [0, 1] },
{ a: [-10, 10], b: [-3, 7] },
{ a: [0, 5], b: [1, 2] },
{ a: [3, 5], b: [3, 4] },
]

const reverseTotalOverlap: RangeTest[] = totalOverlap.map(({ a, b }) => ({ a: b, b: a }))

const noOverlap: RangeTest[] = [
{ a: [0, 1], b: [2, 3] },
{ a: [0, 1], b: [-1, -1] },
{ a: [5, 6], b: [2, 3] },
{ a: [0, 2], b: [-2, -1] },
{ a: [-5, -3], b: [9, 10] },
{ a: [-3, 2], b: [3, 4] },
]

const partialOverlap: RangeTest[] = [
{ a: [0, 3], b: [-1, 1] },
{ a: [0, 1], b: [-1, 0] },
{ a: [0, 0], b: [-1, 0] },
{ a: [0, 2], b: [1, 4] },
{ a: [-8, 0], b: [0, 10] },
]

test.each([...equalRanges, ...totalOverlap])('Range $a should enclose $b', ({ a, b }) =>
expect(rangeEncloses(a, b)).toBe(true),
)
test.each([...noOverlap, ...partialOverlap, ...reverseTotalOverlap])(
'Range $a should not enclose $b',
({ a, b }) => expect(rangeEncloses(a, b)).toBe(false),
)
test.each([...equalRanges, ...totalOverlap, ...reverseTotalOverlap, ...partialOverlap])(
'Range $a should intersect $b',
({ a, b }) => expect(rangeIntersects(a, b)).toBe(true),
)
test.each([...noOverlap])('Range $a should not intersect $b', ({ a, b }) =>
expect(rangeIntersects(a, b)).toBe(false),
)
52 changes: 27 additions & 25 deletions app/gui2/shared/languageServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,14 @@ export class LanguageServer extends ObservableV2<Notifications> {
console.dir(params)
}
return await this.client.request({ method, params }, RPC_TIMEOUT_MS)
} catch (e) {
const remoteError = RemoteRpcErrorSchema.safeParse(e)
} catch (error) {
const remoteError = RemoteRpcErrorSchema.safeParse(error)
if (remoteError.success) {
throw new LsRpcError(new RemoteRpcError(remoteError.data), method, params)
} else if (e instanceof Error) {
throw new LsRpcError(e, method, params)
} else if (error instanceof Error) {
throw new LsRpcError(error, method, params)
}
throw e
throw error
} finally {
if (DEBUG_LOG_RPC) {
console.log(`LS [${uuid}] ${method} took ${performance.now() - now}ms`)
Expand Down Expand Up @@ -402,27 +402,29 @@ export class LanguageServer extends ObservableV2<Notifications> {
retry: <T>(cb: () => Promise<T>) => Promise<T> = (f) => f(),
) {
let running = true
;(async () => {
this.on('file/event', callback)
walkFs(this, { rootId, segments }, (type, path) => {
if (
!running ||
type !== 'File' ||
path.segments.length < segments.length ||
segments.some((seg, i) => seg !== path.segments[i])
)
return
callback({
path: { rootId: path.rootId, segments: path.segments.slice(segments.length) },
kind: 'Added',
const self = this
return {
promise: (async () => {
self.on('file/event', callback)
await retry(async () => running && self.acquireReceivesTreeUpdates({ rootId, segments }))
await walkFs(self, { rootId, segments }, (type, path) => {
if (
!running ||
type !== 'File' ||
path.segments.length < segments.length ||
segments.some((segment, i) => segment !== path.segments[i])
)
return
callback({
path: { rootId: path.rootId, segments: path.segments.slice(segments.length) },
kind: 'Added',
})
})
})
await retry(() => this.acquireReceivesTreeUpdates({ rootId, segments }))
if (!running) return
})()
return () => {
running = false
this.off('file/event', callback)
})(),
unsubscribe() {
running = false
self.off('file/event', callback)
},
}
}

Expand Down
4 changes: 1 addition & 3 deletions app/gui2/shared/languageServerTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,7 @@ export namespace response {
contentRoots: ContentRoot[]
}

export interface FileContents {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was the FileReadResult type, but improperly named. According to docs it does have a nested contents field. So it was probably allright, bar the incorrect name.

https://github.com/enso-org/enso/blob/develop/docs/language-server/protocol-language-server.md#result-5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually i discovered this was incorrect, when i was implementing this:

case LsSyncState.Synchronized: {
this.withState(LsSyncState.Reloading, async () => {
const promise = Promise.all([
this.ls.readFile(this.path),
this.ls.fileChecksum(this.path),
])
this.setLastAction(promise)
const [contents, checksum] = await promise
this.syncFileContents(contents.contents, checksum.checksum)
})
return
}

on that note... i'm not 100% sure about this. it doesn't affect the fix if it's removed - it's entirely possible that I've misunderstood what reload is supposed to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that there is a FileContents type: https://github.com/enso-org/enso/blob/develop/docs/language-server/protocol-language-server.md#filecontents

also note that *Result types were basically only added so that the Result interfaces are valid TS code - in GUI2 these should correspond to response.*

contents: TextFileContents
}
export interface FileContents extends TextFileContents {}

export interface FileExists {
exists: boolean
Expand Down
118 changes: 118 additions & 0 deletions app/gui2/shared/retry.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
import { wait } from 'lib0/promise'

export interface BackoffOptions<E> {
maxRetries?: number
retryDelay?: number
retryDelayMultiplier?: number
retryDelayMax?: number
/** Called when the promise throws an error, and the next retry is about to be attempted.
* When this function returns `false`, the backoff is immediately aborted. When this function
* is not provided, the backoff will always continue until the maximum number of retries
* is reached. * */
onBeforeRetry?: (
error: E,
retryCount: number,
maxRetries: number,
delay: number,
) => boolean | void
/** Called right before returning. */
onSuccess?: (retryCount: number) => void
/** Called after the final retry, right before throwing an error.
* Note that `onBeforeRetry` is *not* called on the final retry, as there is nothing after the
somebody1234 marked this conversation as resolved.
Show resolved Hide resolved
* final retry. */
onFailure?: (error: E, retryCount: number) => void
}

const defaultBackoffOptions: Required<BackoffOptions<unknown>> = {
maxRetries: 3,
retryDelay: 1000,
retryDelayMultiplier: 2,
retryDelayMax: 10000,
onBeforeRetry: () => {},
onSuccess: () => {},
onFailure: () => {},
}

/** Retry a failing promise function with exponential backoff. */
export async function exponentialBackoff<T, E>(
f: () => Promise<T>,
backoffOptions?: BackoffOptions<E>,
): Promise<T> {
const {
maxRetries,
retryDelay,
retryDelayMultiplier,
retryDelayMax,
onBeforeRetry,
onSuccess,
onFailure,
} = {
...defaultBackoffOptions,
...backoffOptions,
}
for (
let retries = 0, delay = retryDelay;
;
retries += 1, delay = Math.min(retryDelayMax, delay * retryDelayMultiplier)
) {
try {
const result = await f()
onSuccess(retries)
return result
} catch (error) {
if (retries >= maxRetries) {
onFailure(error as E, retries)
throw error
}
if (onBeforeRetry(error as E, retries, maxRetries, delay) === false) throw error
await wait(delay)
}
}
}

export function defaultOnBeforeRetry(
description: string,
): NonNullable<BackoffOptions<any>['onBeforeRetry']> {
return (error, retryCount, maxRetries, delay) => {
console.error(
'Could not ' +
description +
` (${retryCount}/${maxRetries} retries), retrying after ${delay}ms...`,
)
console.error(error)
}
}

export function defaultOnFailure(
description: string,
): NonNullable<BackoffOptions<any>['onFailure']> {
return (error, retryCount) => {
console.error(
'Could not ' + description + ` (${retryCount}/${retryCount} retries), throwing error.`,
)
console.error(error)
}
}

export function defaultOnSuccess(
description: string,
): NonNullable<BackoffOptions<any>['onSuccess']> {
return (retryCount) => {
if (retryCount === 0) return
console.info(
'Successfully ' +
description +
` after ${retryCount} ${retryCount === 1 ? 'failure' : 'failures'}.`,
)
}
}

/** @param successDescription Should be in past tense, without an initial capital letter.
* @param errorDescription Should be in present tense, without an initial capital letter. */
export function printingCallbacks(successDescription: string, errorDescription: string) {
return {
onBeforeRetry: defaultOnBeforeRetry(errorDescription),
onSuccess: defaultOnSuccess(successDescription),
onFailure: defaultOnFailure(errorDescription),
} satisfies BackoffOptions<unknown>
}
Loading
Loading