-
Notifications
You must be signed in to change notification settings - Fork 181
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
http-netty: Fix flaky FullDuplexAndSequentialModeTest #3070
http-netty: Fix flaky FullDuplexAndSequentialModeTest #3070
Conversation
Motivation: This test suite has been flaky for a while. As detailed in issue apple#1894, this is because under some conditions the test running thread ends up waiting on a latch that it is actually responsible to release. Modifications: Make sure the publishing of the InputStreamPublisher happens in on a different thread so we don't end up in deadlock. Fixes apple#1894
CountDownLatch continueRequest, | ||
InputStream payload) { | ||
return connection.request(connection.post(SVC_ECHO).payloadBody(fromInputStream(payload, CHUNK_SIZE) | ||
return connection.request(connection.post(SVC_ECHO).payloadBody(fromInputStream(payload, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did the CHUNK_SIZE change from 1024 to 1 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because if it stays what it was the assertThrows(..)
doesn't happen. I suspect we end up racing the cancellation in the separate thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, knowing the reason, I'm agreed that smaller chunk makes it likely to happen we end up in a deadlock. For the original purpose of the test, it should not matter though. So, I don't mind if we change the size here or not.
servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java
Outdated
Show resolved
Hide resolved
...talk-http-netty/src/test/java/io/servicetalk/http/netty/FullDuplexAndSequentialModeTest.java
Outdated
Show resolved
Hide resolved
CI failures are known flaky tests. Merging. |
Motivation:
This test suite has been flaky for a while. As detailed in issue #1894, this is because under some conditions the test running thread ends up waiting on a latch that it is actually responsible to release.
Modifications:
Make sure the publishing of the InputStreamPublisher happens in on a different thread so we don't end up in deadlock.
Fixes #1894