From d2bda3d415dfac25e00d2d976e63d502f53c9798 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Thu, 6 Jun 2019 16:09:11 -0700 Subject: [PATCH] search: fix regression where search result highlighting is not bailed out after 3s (#4369) * search: fix regression where search result highlighting is not bailed out after 3s Prior to this change there was a major regression in which we would disable the syntax highlighting timeout outright. This is despite the fact that this should only ever be done when the user has communicated an intent to wait longer for search results. This regression meant that when a search turned up files that the syntax highlighter could not highlight quickly, we would just wait for them until it finished. In one customer environment I observed search results taking 30s+ to load due to this because it had the added negative consequence of us not canceling those poorly performing requests which led to syntect_server trying to perform tons of CPU-intensive requests which harmed other requests it was handling. After this change, we are properly bailing out once again after the syntax highlighting timeout. This means that in this bad case you are waiting 3s then getting plaintext results instead of waiting 30s+ for highlighting. Fixes #4268 Test plan: Manually tested using #4364 & added a regression test for one unit --- CHANGELOG.md | 1 + shared/src/components/CodeExcerpt.test.tsx | 23 ++++++++++++++++++++++ shared/src/components/CodeExcerpt.tsx | 2 +- web/src/repo/backend.tsx | 3 --- 4 files changed, 25 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4485c2289b74..af26ba8a078c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ All notable changes to Sourcegraph are documented in this file. - Fixed an issue with `sourcegraph/server` Docker deployments where syntax highlighting could produce `server closed idle connection` errors. [#4269](https://github.com/sourcegraph/sourcegraph/issues/4269) - Repositories containing submodules not on Sourcegraph will now load without error [#2947](https://github.com/sourcegraph/sourcegraph/issues/2947) - HTTP metrics in Prometheus/Grafana now distinguish between different types of GraphQL requests. +- Fixed an issue where syntax highlighting taking too long would result in errors or wait long amounts of time without properly falling back to plaintext rendering after a few seconds. [#4267](https://github.com/sourcegraph/sourcegraph/issues/4267) [#4268](https://github.com/sourcegraph/sourcegraph/issues/4268) ## 3.4.2 diff --git a/shared/src/components/CodeExcerpt.test.tsx b/shared/src/components/CodeExcerpt.test.tsx index bccd98d856a6..c498a74bbcf3 100644 --- a/shared/src/components/CodeExcerpt.test.tsx +++ b/shared/src/components/CodeExcerpt.test.tsx @@ -1,5 +1,6 @@ import * as React from 'react' import _VisibilitySensor from 'react-visibility-sensor' +import sinon from 'sinon' export class MockVisibilitySensor extends React.Component<{ onChange?: (isVisible: boolean) => void }> { constructor(props: { onChange?: (isVisible: boolean) => void }) { super(props) @@ -23,7 +24,9 @@ jest.mock( ) import { cleanup, getAllByText, getByText, render } from 'react-testing-library' +import { of } from 'rxjs' import { + HIGHLIGHTED_FILE_LINES, HIGHLIGHTED_FILE_LINES_REQUEST, HIGHLIGHTED_FILE_LINES_SIMPLE_REQUEST, } from '../../../web/src/search/testHelpers' @@ -104,4 +107,24 @@ describe('CodeExcerpt', () => { expect(span.textContent === 'test') } }) + + it('does not disable the highlighting timeout', () => { + /* + Because disabling the timeout should only ever be done in response + to the user asking us to do so, something that we do not do for + code excerpts because falling back to plaintext rendering is most + ideal. + */ + const fetchHighlightedFileLines = sinon.spy(ctx => of(HIGHLIGHTED_FILE_LINES)) + const highlightRange = [{ line: 12, character: 7, highlightLength: 4 }] + render( + + ) + sinon.assert.calledOnce(fetchHighlightedFileLines) + sinon.assert.calledWithMatch(fetchHighlightedFileLines, { disableTimeout: false }) + }) }) diff --git a/shared/src/components/CodeExcerpt.tsx b/shared/src/components/CodeExcerpt.tsx index b9c23d51a684..5fb076284f4c 100644 --- a/shared/src/components/CodeExcerpt.tsx +++ b/shared/src/components/CodeExcerpt.tsx @@ -66,7 +66,7 @@ export class CodeExcerpt extends React.PureComponent { commitID, filePath, isLightTheme, - disableTimeout: true, + disableTimeout: false, }) ), catchError(error => [asError(error)]) diff --git a/web/src/repo/backend.tsx b/web/src/repo/backend.tsx index c676d848fef9..9e51d03c7f36 100644 --- a/web/src/repo/backend.tsx +++ b/web/src/repo/backend.tsx @@ -215,9 +215,6 @@ export const fetchHighlightedFileLines = memoizeObservable( if (result.isDirectory) { return [] } - if (result.highlightedFile.aborted) { - throw new Error('aborted fetching highlighted contents') - } let parsed = result.highlightedFile.html.substr(''.length) parsed = parsed.substr(0, parsed.length - '
'.length) const rows = parsed.split('')