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.

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

Jump to bottom

Evict connection pool a second time after tests #7819

Merged
merged 3 commits into from
May 14, 2023
Merged

Evict connection pool a second time after tests #7819

merged 3 commits into from
May 14, 2023

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented May 13, 2023 •
edited
Loading

Debugging flakyness introduced in #7815

@yschimke yschimke changed the title Output failing calls Improve test logging on unclosed connections and evict a second time May 13, 2023
@yschimke yschimke marked this pull request as ready for review May 13, 2023 15:06
@yschimke
Copy link
Collaborator Author

@swankjesse is iterating over the connections and calls safe here?

I think the fix to evict a second time after waiting 500ms extra, is the real fix.

@yschimke yschimke changed the title Improve test logging on unclosed connections and evict a second time Evict connection pool a second time after tests May 14, 2023
@yschimke
Copy link
Collaborator Author

I'm hopeful this fixes about 50% of our flakes, ones of this form.

EventSourceHttpTest > cancelInEventShortCircuits() > okhttp3.sse.internal.EventSourceHttpTest.cancelInEventShortCircuits()[1] FAILED
    org.opentest4j.AssertionFailedError: expected: <0> but was: <1>
        at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
        at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
        at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
        at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:528)
        at okhttp3.OkHttpClientTestRule.ensureAllConnectionsReleased(OkHttpClientTestRule.kt:193)

@yschimke
Copy link
Collaborator Author

yschimke commented May 14, 2023 •
edited
Loading

@swankjesse requesting post review. Fixing master

@yschimke yschimke merged commit e2344c7 into square:master May 14, 2023
yschimke added a commit to yschimke/okhttp that referenced this pull request May 14, 2023
@yschimke
Evict connection pool a second time after tests (square#7819)
bab3c41 
(cherry picked from commit e2344c7)
swankjesse pushed a commit that referenced this pull request Sep 15, 2023
@yschimke
[4.x] Fix websocket reconnect race condition (#7815) (#7817)
f04809d 
* Fix websocket reconnect race condition (#7815)

(cherry picked from commit f62fd47)

* Attempt to fix a flaky test

* Evict connection pool a second time after tests (#7819)

(cherry picked from commit e2344c7)

* Simplify
@yschimke yschimke deleted the output_calls branch October 15, 2023 11:54
swankjesse
swankjesse reviewed Dec 9, 2023
View reviewed changes
Copy link
Member

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swankjesse swankjesse swankjesse left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yschimke @swankjesse