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 #3957: Lister onOpen should be called before marking the connection open #3958

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

AdrianFarmadin
Copy link
Contributor

@AdrianFarmadin AdrianFarmadin commented Mar 10, 2022

Description

Fix #3957

In PodOperationsImpl class the buildAsync method is called with ExecWebSocketListener.
The ExecWebSocketListener is opening outputPipe and errorPipe in onOpen.
If the future is completed before the outputPipe and errorPipe are open, then the consumer can read from the pipes before they are open.

The pipes should be open first and then the future should be marked as complete.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@shawkins shawkins added this to the 6.0.0 milestone Mar 10, 2022
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

LGTM - but this primarily needs applied to master and can there be an update to the javadocs of our WebSocket.Builder.buildAsync to explicitly state the expectation that onOpen is called first - just to make sure that if for whatever reason we can't honor that we'll have to change the calling logic.

This happened because of eliminating the additional wait logic that was in the ExecWebSocketListener, so this is applicable to 5.11 as well.

The JDK client behavior is consistent with this change - the returned future from buildAsync will not be completed until after onOpen has been called (but that is not explicitly called out in the javadocs).

@AdrianFarmadin
Copy link
Contributor Author

I added javadocs.

Should I create pull requests for master and 5.11 as well?

@shawkins shawkins self-requested a review March 14, 2022 22:43
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

LGTM

@manusa how do you usually handle porting to other branches - is that something that you usually just cherry-pick and update from there?

@manusa
Copy link
Member

manusa commented Mar 22, 2022

LGTM

@manusa how do you usually handle porting to other branches - is that something that you usually just cherry-pick and update from there?

I switched this to master and added the backport label. I'm still not sure how I'm going to handle these ones, since some of the selected changes are going to be hard to move to 5.12.

@sonarcloud
Copy link

sonarcloud bot commented Mar 22, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@manusa manusa merged commit eaf7a15 into fabric8io:master Mar 22, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this pull request Mar 23, 2022
fix fabric8io#3995: addressing hung tests

essentially one thread holding the reflector lock was waiting for the
watch to start.  Another thread via onOpen was trying to stop the
reflector.  I don't believe this happened before fabric8io#3958

the fix is to use an async watch start
shawkins added a commit to shawkins/kubernetes-client that referenced this pull request Mar 23, 2022
essentially one thread holding the reflector lock was waiting for the
watch to start.  Another thread via onOpen was trying to stop the
reflector.  I don't believe this happened before fabric8io#3958

the fix is to use an async watch start
shawkins added a commit to shawkins/kubernetes-client that referenced this pull request Mar 23, 2022
essentially one thread holding the reflector lock was waiting for the
watch to start.  Another thread via onOpen was trying to stop the
reflector.  I don't believe this happened before fabric8io#3958

the fix is to use an async watch start
@manusa manusa removed the 5.12.x Backportable tentative label Apr 5, 2022
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.

PodOperationsImpl should call listener.onOpen before marking the future as complete
4 participants