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 unhandled promise rejection during git operations #12433

Merged

Conversation

erezmus
Copy link
Contributor

@erezmus erezmus commented Apr 19, 2023

What it does

This fixes an issue we've seen happening on KSC when there's a conflict while pulling git changes from a remote.

It seems that when exception is thrown from the dugite-git library, an unhandled promise rejection is caught by the 'unhandledRejection' handler which rethrows the error and ends up in the 'uncaughtException' handler, which caused the container to crash on kubernetes. On a local docker compose setup, the container doesn't crash.

How to test

  • Clone a repository in theia
  • In another clone of the same repository, add some extra commits
  • Back in theia, change one of the files from previous step
  • Pull the changes, it should give you an error message of the conflict
  • In the logs, the following should not be visible:
2023-04-19T13:35:48.732Z root ERROR Uncaught Exception:  GitError: Your local changes to the following files would be overwritten

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the git issues related to git label Apr 19, 2023
@vince-fugnitto vince-fugnitto modified the milestone: 1.37.0 Apr 27, 2023
@tsmaeder
Copy link
Contributor

@erezmus wondering why this would crash the container on Kubernetes. I have a hard time to imagine why that would be the case.

@erezmus
Copy link
Contributor Author

erezmus commented May 31, 2023

@tsmaeder not sure why it happened in our cluster but after applying that patch in our theia fork, it fixed the problem 🤷

@@ -30,7 +30,7 @@ export class GitRepositoryManager {

run<T>(repository: Repository, op: () => Promise<T>): Promise<T> {
const result = op();
result.then(() => this.sync(repository));
result.then(() => this.sync(repository)).catch(e => console.log(e));
Copy link
Member

Choose a reason for hiding this comment

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

Only the sync is actually unhandled, whereas the op() call is handled at a higher level. If we use a catch-all mechanism here, we potentially swallow errors that should be thrown and caught on the caller site. We should only call catch on the sync promise:

Suggested change
result.then(() => this.sync(repository)).catch(e => console.log(e));
result.then(() => this.sync(repository).catch(e => console.log(e)));

@msujew
Copy link
Member

msujew commented Aug 14, 2023

@erezmus Are you still interested in contributing the fix? I have an outstanding comment that I would like to see addressed before we can merge this.

If there's no further feedback, I would close the PR in the next few days.

@msujew msujew closed this Aug 14, 2023
@msujew msujew reopened this Aug 14, 2023
@CareyJWilliams
Copy link
Contributor

@erezmus Are you still interested in contributing the fix? I have an outstanding comment that I would like to see addressed before we can merge this.

If there's no further feedback, I would close the PR in the next few days.

@erezmus won't be available until the end of August.

@erezmus
Copy link
Contributor Author

erezmus commented Aug 29, 2023

@msujew i'll try to have a look today or tomorrow, otherwise next week

@erezmus
Copy link
Contributor Author

erezmus commented Aug 30, 2023

@msujew I've had a test both on theia and ksc with your adopted change, looks good

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Awesome, looks good to me as well 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git issues related to git
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants