You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/04/24 21:52:10 UTC
[GitHub] [thrift] penenin opened a new pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
penenin opened a new pull request #2121:
URL: https://github.com/apache/thrift/pull/2121
<!-- Explain the changes in the pull request below: -->
Add support for using WebSockets as a server transport to the C++ library.
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [X] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [X] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [X] Did you squash your changes to a single commit? (not required, but preferred)
- [X] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, add ` [skip ci]` at the end of your pull request to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-628630691
Since I had already modified lib/nodejs/test/client.js, I went ahead and modified server.js too. Everything passes or has been marked as an expected failure as appropriate. I believe this should be ready to merge, but let me know if I need to make more changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-630544893
AFAIK, there aren't any standard functions to byteswap 64-bit integers. We need something to paper over that difference at least. I thought including this file would be okay because it states you can use it under the Apache license. I can replace it with something that just defines ntohll() and use that family of functions in a separate PR if you wish. Let me know what you would like me to do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-621373589
I hadn't seen that. If I had, I may not have written this code. I am working on a project where I am not completely sure whether I will be able to utilize D (my first choice) or not. I ported my D implementation to C++ just in case. I was trying to figure out how to add tests for this PR, but if there is a better, more mature implementation ready to upstream, I can always close this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun merged pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
dcelasun merged pull request #2121:
URL: https://github.com/apache/thrift/pull/2121
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G edited a comment on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-632394608
> Could that be used instead?
I had the same thoughs ... we have code for it. Shouldn't that be sufficient?
> the definition of 'htonll' breaks the build for me
Actually, on the Github page of said header, someone also commented that MSVC seems not really good covered by it, at least this was the case in 2018.
https://gist.github.com/panzi/6856583#gistcomment-2396000
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#discussion_r415563324
##########
File path: lib/cpp/src/thrift/endian.h
##########
@@ -0,0 +1,124 @@
+//
Review comment:
Sorry I thought that you were the author. As per [ASF policy](https://www.apache.org/legal/resolved.html#handling-public-domain-licensed-works), let's keep the existing text.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
dcelasun commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-629452939
Thanks for working on this! That's quite the list of known failures, but I'm not really familiar with websockets so if you and @emmenlau think they are ok, it's fine by me.
Unfortunately, tests are still broken and they are cpp related. See [here](https://travis-ci.org/github/apache/thrift/jobs/686855310) and [here](https://travis-ci.org/github/apache/thrift/jobs/686855311).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-623896082
It would be sad to loose your changes!
The test suite is working to some extend via cmake. Can you tell me in more detail which tests would be relevant for you? And is the linker error visible in the CI here, so I could try to reproduce it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-629471074
> Unfortunately, tests are still broken and they are cpp related. See [here](https://travis-ci.org/github/apache/thrift/jobs/686855310) and [here](https://travis-ci.org/github/apache/thrift/jobs/686855311).
I've tried to follow up the second error and https://www.reddit.com/r/gcc/comments/4lcsfj/gcc_error_in_compilation/ seems related. It claims that messing with and of the following variables may have something to do with it: `__BYTE_ORDER`, `__LITTLE_ENDIAN`, `__BIG_ENDIAN`. Is it possible that these get defined in Thrift, or possibly included via `TProtocol.h:91:# include <boost/predef/other/endian.h>`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-632389755
That is weird. I haven't run into that and the PR passed CI. Regardless, it might be moot if I am removing portable_endian.h. I will look at THRIFT_htonll and open a new PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-629753030
The failing Travis test looks to be a Python issue. There is still a linker error on Windows that AppVeyor is reporting. I will try to reproduce.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-632765278
Changes submitted as #2152
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-624900051
I finally tracked the bug down. I was accessing uninitialized variables. I have added tests for a C++ server and NodeJS client. All the test pass except for tests with the domain socket transport and tests with the header protocol. I have marked them as expected failures.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-632360470
Actually I've also seen this too late. The definition of 'htonll' breaks the build for me on VS2017 and VS2019 when building Thrift TestServer.cpp:
```
thrift\lib\cpp\src\thrift/transport/TWebSocketServer.h(366): error C3861: 'htonll': identifier not found
```
I've tried a number of things to work around this issue but as far as I can see its not trivial. I've moved a number of `Windows.h` includes out of the way which supposedly break including `WinSock2.h`, but that did not help. Defining `WIN32_LEAN_AND_MEAN` globally also did not resolve the problem. And shuffling the headers to that `TWebSocketServer.h` comes first also does not help. So I'm at the end of my wit, suggestions welcome.
Also, did you see that `TProtocol.h` has a defintion of `THRIFT_htonll`? Could that be used instead?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-621392545
Sadly, the linked implementation is much harder to upstream than yours, so I've seen this PR with some delight. The main benefit of my solution is that it includes the full webserver (HTTP + HTTPS) as well as the full websocket server (again plain + SSL supported), all on the same port. This is very useful for web clients that require the HTTP server and websocket server on the same source (or cross-site scripting protection may kick in). I'm not sure if this is relevant for you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
dcelasun commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-630389792
The Python issue will be fixed by #2147. Thanks for working on this!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on a change in pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on a change in pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#discussion_r415449352
##########
File path: lib/cpp/src/thrift/endian.h
##########
@@ -0,0 +1,124 @@
+//
Review comment:
Since I didn't author the file, but it states that it is in the public domain, or can be licensed under Apache 2.0, should I put the standard Apache license header in addition to the existing text or are you asking me to replace the existing text?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-630388065
Everything is passing except "Python code style" in the last Travis test. Since that doesn't have anything to do with C++, the PR should be ready to merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-621358923
@penenin I'm curious if you've seen our HTTP and Thrift-WebSocket server at https://github.com/BioDataAnalysis/thrift-http-ws-server? Its based on boost::beast so it does not directly relate to your code, but I still wanted to point it out...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-624138820
I think I confused things with my comments above. The linker error I encountered was in D on Linux. To try to clarify, this is what I have done and where I am at:
- I manually tested my code on Windows by modifying the C++ tutorial server and the nodejs tutorial client. I bundled the client using Webpack and was able to communicate from Firefox and Chrome to the tutorial server over WebSockets.
- I opened the PR and was asked to provide tests.
- On Linux, I modified the C++ cross language test server, nodejs test client, and js test client to add websockets and accept websocket as a command line argument.
- None of the tests work. When I look at the generated network traffic with Wireshark, I don't see the WebSocket handshake like I expect.
- Then still on Linux, I recreated my modified C++ tutorial server and Webpacked tutorial client. Those don't work either and I don't see the WebSocket traffic in Wireshark like I would expect.
- I then tried to modified the D tutorial server on Linux to test WebSockets in order to see if I was running into a Linux platform issue or if there was a difference in behavior between the two language libraries. That is where I ran into the linker error I don't see in Windows.
Setting aside the last point about D as off-topic since this is a C++ PR, the only thing I can think of is that I am running into some difference between the Linux and Windows socket implementations, or that there is some sort of compression going on that I am not aware of (even though I have tried to turn off per message deflate). Since my Linux development experience is very limited, the biggest hurdle I am running into is trying to get a good debugging environment set up or just even knowing roughly what I should be looking for. I have been struggling with plain command line gdb.
Based on your suggestion, using CMake, I am trying to get the cross language tests set up and building on Windows, but I am running into build issues there as well.
I can update the PR with the failing tests if you would like to take a look at what I have.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-625078304
Awesome!!!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-632394608
> Could that be used instead?
I had the same thoughs ... we have code for it. Should'nt that be sufficient?
> he definition of 'htonll' breaks the build for me
Actually, on the Github page of said header, someone also commented that MSVC seems not really good covered by it, at least this was the case in 2018.
https://gist.github.com/panzi/6856583#gistcomment-2396000
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G edited a comment on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-632394608
> Could that be used instead?
I had the same thoughs ... we have code for it. Shouldn't that be sufficient?
> the definition of 'htonll' breaks the build for me
Actually, on the Github page of said header, someone also commented that MSVC seems not really good covered by it, at least this was the case in 2018.
https://gist.github.com/panzi/6856583#gistcomment-2396000
> Otherwise an addition to LICENSE seems appropriate.
I did that a few minutes ago. If we are going to remove the file, please also remove my additions from LICENSE .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] penenin commented on pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
penenin commented on pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#issuecomment-622597856
I can see where that would be useful in some circumstances, but it isn't relevant for my particular project.
I may have to close this PR as the code appears to work on Windows, but not on Linux. I manually tested both this implementation and the D one on Windows with a modified tutorial server and a modified nodejs client bundled for the browser with Webpack. Unfortunately, on Linux, the C++ server won't even complete the WebSocket handshake with my browser client and I can't get my modified D tutorial server to build because of a linker error. It also looks to me like the test suite is Linux only as well. I am primarily a Windows developer and I am running out of things I know to try. I will keep chipping away at it, but I am not sure how long it is going to take me to figure it out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dcelasun commented on a change in pull request #2121: THRIFT-5185: Add support for using WebSockets as a server transport.
Posted by GitBox <gi...@apache.org>.
dcelasun commented on a change in pull request #2121:
URL: https://github.com/apache/thrift/pull/2121#discussion_r414897662
##########
File path: lib/cpp/Makefile.am
##########
@@ -87,6 +87,7 @@ libthrift_la_SOURCES = src/thrift/TApplicationException.cpp \
src/thrift/transport/TNonblockingSSLServerSocket.cpp \
src/thrift/transport/TTransportUtils.cpp \
src/thrift/transport/TBufferTransports.cpp \
+ src/thrift/transport/TWebSocketServer.cpp \
Review comment:
nit: Fix alignment
##########
File path: lib/cpp/src/thrift/endian.h
##########
@@ -0,0 +1,124 @@
+//
Review comment:
This header is likely not acceptable for ASF. If you have no objections, please use the standard license header.
##########
File path: lib/cpp/Makefile.am
##########
@@ -198,7 +200,8 @@ include_transport_HEADERS = \
src/thrift/transport/TTransportUtils.h \
src/thrift/transport/TBufferTransports.h \
src/thrift/transport/TShortReadTransport.h \
- src/thrift/transport/TZlibTransport.h
+ src/thrift/transport/TZlibTransport.h \
+ src/thrift/transport/TWebSocketServer.h
Review comment:
nit: Fix alignment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org