You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by chetmurthy <gi...@git.apache.org> on 2017/11/27 00:45:51 UTC
[GitHub] thrift pull request #1418: Thrift 3877 http server buffering bug oneway
GitHub user chetmurthy opened a pull request:
https://github.com/apache/thrift/pull/1418
Thrift 3877 http server buffering bug oneway
This PR is in two commits, so that the reviewer can
(a) apply the testcase commit, elicit the failure by running lib/cpp/tests/UnitTests
(b) apply the (full) second commit, rerun testcase, and see the problem is gone.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chetmurthy/thrift THRIFT-3877-http-server-buffering-bug-oneway
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1418.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1418
----
commit 7d83cb4a2cafe033e7d18705cc0f74cd9b883cd5
Author: Chet Murthy <ch...@gmail.com>
Date: 2017-11-27T00:39:15Z
THRIFT-3877 unit-test elicits failure of C++/HTTP/oneway hang
Hit a C++ HTTP server with a oneway rpc, and the next RPC will hang.
This test-case elicits the failure (converts to timeout-expiry).
commit 10c8951de5f483b9bc71e2aa1558dd0170389bce
Author: Chet Murthy <ch...@gmail.com>
Date: 2017-11-27T00:42:28Z
THRIFT-3877 this fixes the subject bug
C++ HTTP server, hit with oneway RPC, then roundtrip RPC, no longer
hangs, as demonstrated by OneWayHTTPTest.
----
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by chetmurthy <gi...@git.apache.org>.
Github user chetmurthy commented on the issue:
https://github.com/apache/thrift/pull/1418
This patch only flxes cpp-cpp (http) failures. B/c the deeper issue is that the Thrift cpp stack (both client & server) is nonstandard.
I think I mentioned that I could improve this a bit, by having the client not wait for the reply from the server, but the server would eventually deliver it anyway. The client would keep track of of how many replies it had ignored, and when it came time to actually want to see a reply, it woud skip past the requisite # of ignored replies. Also (of course), you'd want to only ignore replies up to some limit -- or you could deadlock.
Not very hard to do, actually. And this would also fix cpp(client)-http-<anylang>(server) tests. BUT it would NOT fix <anylang>(client)-http<anylang>(server) tests, b/c (from what I remember of my little stroll thru code) (almost?) all the other languages' HTTP stacks assume an RPC model for how applications will use the client HTTP stack. So they're written with "send request, immediately turn around and wait for response" as the model.
Hope this helps.
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by chetmurthy <gi...@git.apache.org>.
Github user chetmurthy commented on the issue:
https://github.com/apache/thrift/pull/1418
I looked at the AppVeyor logs failing jobs, and cannot see why they failed. It looks like they just ran outta time? I have no idea what to do to get these jos to re-run -- could someone enlighten me?
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by chetmurthy <gi...@git.apache.org>.
Github user chetmurthy commented on the issue:
https://github.com/apache/thrift/pull/1418
OK, I see that the Travis CMake builds all failed. I figured out how to run cmake on my laptop, but .... I have no idea how to add the dependency that it's asking for.
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by chetmurthy <gi...@git.apache.org>.
Github user chetmurthy commented on the issue:
https://github.com/apache/thrift/pull/1418
OK, I figured out how to fix cmakelists.txt, to get most of the cmake builds working. But one still fails. Then I tried to set up and run docker, to debug that. And can't figure out how from the instructions here: https://github.com/apache/thrift/blob/master/build/docker/README.md
to no avail.
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by chetmurthy <gi...@git.apache.org>.
Github user chetmurthy commented on the issue:
https://github.com/apache/thrift/pull/1418
Will do. Please do suggest any style changes that you think appropriate -- I am *fully* bought-into the idea that a project should have a uniform style, and even if I were not to agree with that style, I'd go along with it, b/c ... uniformity has value.
To that end, should I add a block-comment to the test, explaining what the classes do, what's the idea is behind it, etc?
---
[GitHub] thrift pull request #1418: Thrift 3877 http server buffering bug oneway
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1418
---
[GitHub] thrift pull request #1418: Thrift 3877 http server buffering bug oneway
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1418#discussion_r153869127
--- Diff: test/ThriftTest.thrift ---
@@ -409,3 +409,8 @@ struct StructB {
1: optional StructA aa;
2: required StructA ab;
}
+
+service OneWayTest {
--- End diff --
Could we move OneWayTest to its own thrift file, which could be located inside lib/cpp/test because it is only used for that unit test? I can help in making sure the Makefile.am and the CMakeLists.txt are updated appropriately.
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1418
Pulling this into a sandbox to see if it fixes any of the `cpp*http` known test failures.
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by chetmurthy <gi...@git.apache.org>.
Github user chetmurthy commented on the issue:
https://github.com/apache/thrift/pull/1418
Right, the "docker run" command fails with:
chet@twitter:~/Hack/thrift-0.10.0/src/forks/thrift$ dockerrun ubuntu-xenial
Unable to find image 'ubuntu-xenial:latest' locally
docker: Error response from daemon: repository ubuntu-xenial not found: does not exist or no pull access.
See 'docker run --help'.
chet@twitter:~/Hack/thrift-0.10.0/src/forks/thrift$
the "refresh.sh" cmd worked fine.
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1418
@chetmurthy try "Raw Commands for Building with Docker" at that page.
Once you are in a docker container:
build/docker/scripts/autotools.sh runs a "make check" build
build/docker/scripts/crosstest.sh runs a "make cross" build
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1418
I think it's fine as-is, except for adding to ThriftTest in this case since that is a global test thrift file, and you need one local to lib/cpp/test. The fact you took the time to add a unit test and did it in a very TDD way makes me quite happy. :)
---
[GitHub] thrift issue #1418: Thrift 3877 http server buffering bug oneway
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1418
I'm not certain this resolves THRIFT-3877 entirely - your take?
---