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

fix(inline-suggestion): replace vscode.cancellation with waitUntil for timeout #6256

Merged
merged 7 commits into from
Dec 16, 2024

Conversation

Will-ShaoHua
Copy link
Contributor

@Will-ShaoHua Will-ShaoHua commented Dec 16, 2024

Problem

related issue: #6079, #6252

caller

function main () {
       // init vscode cancellation token
       const cancellationToken
       setTimeout(100, () => {
              cancellationToken.cancel()
       })

      highlevelWrapperFetchSupplementalContext(editor, cancellationToken)
}

export function highlevelWrapperFetchSupplementalContext(editor, cancellationToken) {
       const supplementalContext = waitUntil(100, () => {
            // here always timeout and throw TimeoutException
            const opentabs =  await fetchOpenTabsContext(...)
            const projectContext = await fetchProjectContext()

           const result = []
           if (projectContext not empty) {
                    // push project context
           }

           if (opentabs not empty) {}
                   // push openttabs
           })  
          

         return result
}


async function fetchOpenTabsContext(editor, cancellationToken) {
      ....
      // VSC api call
}

async function fetchProjectContext() {
     ....
     // LSP call
}

After investigation, it looks like mix use of vscode.CancellationToken and waitUntil() will likely cause cancellation token to be cancelled prematurely (might be because another layer of waitUntil will run the fetchOpenTabsContext asynchronously thus causing it to timeout prematurely) therefore fetchOpebtabsContext(..) will return null in this case and hence causing test cases failing.

Therefore, the issue here is actually not the test case itself and they're failing due to race condition

Solution

remove usage of cancellation token and only use waitUntil for timeout purpose

Functional testing

retrieved sup context as expected

Case 1: repomap is available (there are local imports)

2024-12-16 13:10:15.616 [debug] CodeWhispererSupplementalContext:
    isUtg: false,
    isProcessTimeout: false,
    contentsLength: 14436,
    latency: 16.67179101705551
    strategy: codemap
    Chunk 0:
        Path: q-inline
        Length: 10209
        Score: 0
    Chunk 1:
        Path: /Volumes/workplace/ide/aws-toolkit-vscode-staging/packages/core/src/codewhisperer/service/serviceContainer.ts
        Length: 1486
        Score: 22.60257328587725
    Chunk 2:
        Path: /Volumes/workplace/ide/aws-toolkit-vscode-staging/packages/core/src/codewhisperer/tracker/lineTracker.ts
        Length: 1649
        Score: 19.106700952807103
    Chunk 3:
        Path: /Volumes/workplace/ide/aws-toolkit-vscode-staging/packages/core/src/codewhisperer/tracker/lineTracker.ts
        Length: 1092
        Score: 10.334690655691002

Case 2: No repomap, should fallback to opentabs only

image

2024-12-16 13:11:29.738 [debug] CodeWhispererSupplementalContext:
    isUtg: false,
    isProcessTimeout: false,
    contentsLength: 5046,
    latency: 16.311500012874603
    strategy: opentabs
    Chunk 0:
        Path: /Volumes/workplace/ide/aws-toolkit-vscode-staging/packages/core/src/codewhisperer/tracker/lineTracker.ts
        Length: 1564
        Score: 0
    Chunk 1:
        Path: /Volumes/workplace/ide/aws-toolkit-vscode-staging/packages/core/src/codewhisperer/tracker/lineTracker.ts
        Length: 1649
        Score: 0
    Chunk 2:
        Path: /Volumes/workplace/ide/aws-toolkit-vscode-staging/packages/core/src/codewhisperer/tracker/lineTracker.ts
        Length: 1833
        Score: 0

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Will-ShaoHua Will-ShaoHua requested a review from a team as a code owner December 16, 2024 20:43

This comment was marked as resolved.

@Will-ShaoHua Will-ShaoHua requested a review from a team as a code owner December 16, 2024 20:48
@@ -44,7 +44,7 @@ describe('crossFileContextUtil', function () {
sinon.restore()
})

it('for control group, should return opentabs context where there will be 3 chunks and each chunk should contains 50 lines', async function () {
it.skip('for control group, should return opentabs context where there will be 3 chunks and each chunk should contains 50 lines', async function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we always assume t1 group for now

@Will-ShaoHua
Copy link
Contributor Author

looking into test failure

@Will-ShaoHua Will-ShaoHua merged commit ad52466 into aws:master Dec 16, 2024
24 of 26 checks passed
@Will-ShaoHua Will-ShaoHua deleted the timeout-fix branch December 16, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants