Skip to content

fix(amazonq): fix connection manager deadlock when q ListAvailableProfiles is long #5547

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 9, 2025
Merged

fix(amazonq): fix connection manager deadlock when q ListAvailableProfiles is long #5547

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a69c852
fix(amazonq): fix connection manager deadlock when q ListAvailablePro…
rli Apr 9, 2025
612419f
feedback
rli Apr 9, 2025
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
4 changes: 4 additions & 0 deletions .changes/next-release/bugfix-1162ea20-1fcf-43a6-b502-16d8b7d588d7.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type" : "bugfix",
"description" : "Fix issue where IDE freezes when logging into Amazon Q"
}
5 changes: 4 additions & 1 deletion ...c/software/aws/toolkits/jetbrains/services/amazonq/toolwindow/AmazonQToolWindowFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package software.aws.toolkits.jetbrains.services.amazonq.toolwindow

import com.intellij.openapi.actionSystem.ActionManager
import com.intellij.openapi.application.ApplicationManager
import com.intellij.openapi.application.runInEdt
import com.intellij.openapi.project.DumbAware
import com.intellij.openapi.project.Project
Expand Down Expand Up @@ -65,7 +66,9 @@
override fun activeConnectionChanged(newConnection: ToolkitConnection?) {
ToolkitConnectionManager.getInstance(project).activeConnectionForFeature(QConnection.getInstance())?.let { qConn ->
openMeetQPage(project)
QRegionProfileManager.getInstance().validateProfile(project)
ApplicationManager.getApplication().executeOnPooledThread {
QRegionProfileManager.getInstance().validateProfile(project)
Copy link
Contributor

@Will-ShaoHua Will-ShaoHua Apr 9, 2025

Choose a reason for hiding this comment

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

i thoguht we already had this in project startup https://github.com/aws/aws-toolkit-jetbrains/blob/main/plugins/amazonq/chat/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/startup/AmazonQStartupActivity.kt#L57

Copy link
Contributor

@Will-ShaoHua Will-ShaoHua Apr 9, 2025

Choose a reason for hiding this comment

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

@evanliu048

Copy link
Contributor

@Will-ShaoHua Will-ShaoHua Apr 9, 2025

Choose a reason for hiding this comment

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

do we really need this ?

Copy link
Contributor Author

@rli rli Apr 9, 2025

Choose a reason for hiding this comment

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

both --
so if we are checking in project startup, do we still need this one then?

Copy link
Contributor

@Will-ShaoHua Will-ShaoHua Apr 9, 2025

Choose a reason for hiding this comment

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

i feel we shouldn't need this call at all

Copy link
Contributor

@evanliu048 evanliu048 Apr 9, 2025

Choose a reason for hiding this comment

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

I think we might need to recheck the behavior when a user logs out and logs back in with completely different profiles. From my testing, validating only during project startup doesn't seem to be sufficient. Please feel free to correct me if I’m missing something!

Copy link
Contributor

@Will-ShaoHua Will-ShaoHua Apr 9, 2025

Choose a reason for hiding this comment

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

implement it in a different way

  1. log out clear the connection to profile arn cache, because anyway when you login, you have to reselect the profile
  2. remove validateConnection onConnectionChanged

Copy link
Contributor

@evanliu048 evanliu048 Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see the reasoning behind clearing the cache on logout. That said, I think we still need to call listProfiles inside onConnectionChanged. Otherwise, prepareChatContent won’t be able to determine whether to open the WebView or Q panel. Let me know what you think.
https://github.com/aws/aws-toolkit-jetbrains/blob/main/plugins/amazonq/chat/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/toolwindow/AmazonQToolWindowFactory.kt#L126-L127

Copy link
Contributor Author

@rli rli Apr 9, 2025

Choose a reason for hiding this comment

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

yes the list on prepareChatContent is separate from what this PR is addressing

}

Check warning on line 71 in plugins/amazonq/chat/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/toolwindow/AmazonQToolWindowFactory.kt

View check run for this annotation

Codecov / codecov/patch

plugins/amazonq/chat/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/toolwindow/AmazonQToolWindowFactory.kt#L69-L71 

Added lines #L69 - L71 were not covered by tests
}
prepareChatContent(project, qPanel)
}
Expand Down
2 changes: 2 additions & 0 deletions ...ity/src/software/aws/toolkits/jetbrains/services/amazonq/profile/QRegionProfileManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import com.intellij.openapi.components.State
import com.intellij.openapi.components.Storage
import com.intellij.openapi.components.service
import com.intellij.openapi.project.Project
import com.intellij.util.concurrency.annotations.RequiresBackgroundThread
import com.intellij.util.xmlb.annotations.MapAnnotation
import com.intellij.util.xmlb.annotations.Property
import software.amazon.awssdk.core.SdkClient
Expand Down Expand Up @@ -43,6 +44,7 @@ class QRegionProfileManager : PersistentStateComponent<QProfileState>, Disposabl
private val connectionIdToProfileList = mutableMapOf<String, Int>()

// should be call on project startup to validate if profile is still active
@RequiresBackgroundThread
fun validateProfile(project: Project) {
val conn = getIdcConnectionOrNull(project)
val selected = activeProfile(project) ?: return
Expand Down
Loading