Hey guys, sorry for the late response. I've been heads down implementing anonymous pipe transport and now have that working in the TPipe / TPipeServer classes. Now that I've got something working I can start mapping that onto cross-platform code. I'm still digesting Rush's Asio implementation and agree it's an impressive body of work. I may have to take some of this discussion offline with Rush & Alex to try to get this pulled together. For instance I'm still trying to understand why a sockets style transport is used for the local transports. The Windows Pipe transports are a much better fit in the file type of transports. There are significant differences though and a number of Windows-specific calls that I'm afraid can't avoid #ifdef's.
-Peace ________________________________ From: Rush Manbert <[email protected]> To: [email protected] Sent: Tuesday, December 6, 2011 4:29 PM Subject: Re: Pipe transport for Windows Hi Alex, Replies inline. On Dec 5, 2011, at 10:13 PM, Alex Parenteau wrote: > I was looking at the patch, and it is fantastic! Very inlined with what I > would have hoped for thrift (i.e. a boost::asio implementation) Gosh, thanks. I'm blushing. ;-) > > Me wondering whether re-submitting the patch, on top of actual thrift trunk, > would have more chances to go in this time. I did not read yet the arguments > that lead to suspend it, but from what I see the boost::asio implementation > is fairly large, and would benefit IMHO to: I think the JIRA incident contains a long, extended conversation about the patch contents. At the time I submitted it, there was no one who could maintain it. I certainly can't. It was also a BIG piece of work to review and digest, and there wasn't anyone else around who needed it enough to take that on. I fixed bugs in places that were unexpected, like the threading implementation, and that made the whole thing more "dangerous" in some sense. Eventually, the code base passed it by, and I don't know how hard it would be to resurrect it now. > > - create a parallel structure, for easy integration, and comparison sake > between libevent/threadpool. I'm not sure I understand this comment. Everything I did was done in parallel with the original implementation. The ASIO-based classes are parallel with the original classes, and at config time you decide which implementation you use. The class names do not change, only the underlying implementation. I did it this way because I knew there were users (Facebook) that would not be interested in an ASIO re-implementation. They had honed the *nix code and it had to be undisturbed. > - make it a linux/windows/mac story, not windows-only (or may be I'm > misunderstanding the purpose for the patch) The patched code is indeed a linux/windows/mac story. My goal was to have Mac and Windows, because that's what we support where I work. But a very knowledgable fellow named Bruce Simpson became interested in it just as I was getting to the point where a beta tester would be a good thing. He tested it in a number of Linux environments and helped me a great deal to work through the issues he discovered. > - integrate/enhance the existing thrift test infrastructure with the stress > program you created (as opposed to something else, not sure what I'm saying > here) The stress test existed before I started. I just added capability to it. It's really useful. Is it not available in the current sources? I think I also wrote a pretty good test of the thread scheduler that's in there somewhere. My version probably runs it as part of "make check". > > Let me know if I could help somehow, I would love to work on a github fork on > this matter (may be helping Peace?!), with the goal to present a case to > Roger and al, and all the thrift developers. We would love for this work to be included in the trunk, so we're not forked forever. I know that Thrift 1031 is a version that uses the Windows version of pthreads, but we need something that is unencumbered with LGPL licensing, so the total boost approach works well for us. As I said in my JIRA comment of 7 October 2010, I'm not in a position where I can work on this. I did the original work because the company I work for needed it and I did the work during my normal work day. I really don't have time to work on this outside of work. Steven Knight worked on applying the work to the 0.4.0 code base, but wasn't able to finish. He uploaded a patch for what he had. Richard Swift was interested, but they must have gone some other way, because AFAICT, he disappeared after Oct. 2010. I haver found a couple of bugs in my implementation over the past couple years. I thought I uploaded corrected files, but I don't see them in the JIRA. I could certainly fish them out of our local SVN repository. > > One feature I did not see though, is the usage of thread pool for client > connections: I guess it would not be too hard to implement, but I digress. I implemented all of the clients and servers that existed in the C++ library at the time I did the work. And now it's been more than 2 years since I finished, so my memories of what's there have dimmed somewhat. I would think that if this was resurrected now, the same approach would apply. Just do a parallel implementation of everything the *nix library does. Best regards, Rush > > alex > > -----Original Message----- > From: Rush Manbert [mailto:[email protected]] > Sent: Friday, December 02, 2011 6:07 PM > To: [email protected] > Subject: Re: Pipe transport for Windows > > Hi Peace, > > No, I'm sorry to say that none of that code ever got brought into the trunk. > We use it, and I know of a couple other commercial sites that use it as well, > but that's all. (Although an interesting side effect is that I get occasional > job offer feelers that are obviously motivated by someone knowing about that > code.) > > If you take a look at the 591 JIRA, you will see a file named > "thrift-818530-patched.zip". That has the patch applied to the thrift source > tree at svn rev 818530, which was current when I submitted it. So you can > just download that and unzip it. > > I don't know how well it meshes with the current sources, and if you have > already done the work it's probably not that useful to you anyway, but it's > written in the Thrift style, and is just another socket type that can be used > with the other pieces. We use it all the time in our code. > > One thing I'll mention that might be helpful to you. When I was doing my > Windows port the most useful test I found was the stress test program. It can > really hammer on the server side and it revealed quite a few bugs for me. My > patched code includes a greatly (IMHO) improved version of the stress test > that can use many different server and socket types, including the domain > sockets and named pipe sockets. You'd have to make some changes, because I > put the two types under (IIRC) TLocalSocket or some such, so the code that > uses it can be cross platform. But it might be worth your while to see if it > can help you. I found a lot of strange, undocumented named pipe behaviors > that way. > > Best regards, > Rush > > On Dec 2, 2011, at 12:32 PM, Peace wrote: > >> Rush - Would you mind uploading your individual files (not in patch form) >> relevant to the pipes code? Did you incorporate it into the TSocket >> transport? I appreciate your work in this area and apologize for >> overlooking 591. I may have viewed it a while ago and there has been so much >> activity on the Windows front that I assumed its contents had been >> incorporated by now. >> >> >> Alex - Thanks for the example code and suggesting boost::asio (same to >> Rush). Is your approach intended to create a new server type? >> >> >> >> Architecturally my preference is for an independent pipe transport that can >> be mixed & matched with any server. Well, any server that accepts a Thrift >> transport. With the Windows fix for 'regular' servers (THRIFT-1433), I am >> able to pass either TSocket / TServerSocket -or- TPipe / TServerPipe >> transports to the same TThreadPoolServer (TPipe is my unsubmitted named pipe >> transport). It just works by leveraging Thrift's layered architecture. >> In the process of creating the pipe transport, I spent a good amount of time >> wading through the T[Server]Socket code trying to understand how the >> callbacks were processed. The TPipe transport is far less wordy by >> comparison. Cramming windows pipes into the existing TSocket transport would >> add to its complexity making it that much more difficult to maintain & >> debug. I'm not intimately familiar with programming Unix domain sockets but >> at a high level it seems to mesh directly with sockets calls. It would make >> sense for that to stay there since it leverages so much of the TSocket >> code. Windows named pipes are quite a different beast though and TSocket >> isn't the best fit. >> >> >> I've been fortunate to be able to take the time to work on pipes but work >> priorities will change soon. I'd hate to see this implementation fall by the >> wayside as it seems to be working well and is cleanly partitioned from other >> modules. >> >> >> -Peace >> >> >> >> ________________________________ >> From: Rush Manbert <[email protected]> >> To: [email protected] >> Cc: 'Peace' <[email protected]> >> Sent: Friday, December 2, 2011 1:04 PM >> Subject: Re: Pipe transport for Windows >> >> I don't want to be snarky, but you guys who are interested in having WIndows >> libraries are slowly reproducing the work I already did and submitted as >> part of https://issues.apache.org/jira/browse/THRIFT-591. >> >> This includes a full ASIO based implementation of local sockets, implemented >> as Unix domain sockets on *nix, and Windows named pipes on Windows. It won't >> apply against the current sources, but the named pipe stuff was all new code >> anyway. >> >> - Rush >> >> On Dec 2, 2011, at 10:47 AM, Alex Parenteau wrote: >> >>> Hi Peace, >>> >>> I would use boost::asio, and use the ability of it to handle Windows/Posix >>> file descriptors. >>> >>> Below are some snippets of code that might help illustrate this (don't try >>> to compile!) >>> >>> I don't know much of the pipe transport in thrift, so this may not be >>> applicable to your question. >>> >>> As an orthogonal thought, I was wondering if anybody has put some thought >>> around using boost::asio for async thrift servers (right now using >>> libevent). >>> >>> Regards, >>> alex >>> >>> #include <boost/asio.hpp> >>> #include <boost/system/windows_error.hpp> >>> >>> #ifdef DVA_OS_WIN >>> using boost::asio::windows::stream_handle; >>> typedef stream_handle platform_stream; >>> typedef HANDLE platform_descriptor; >>> # define PIPE_EOF_ERROR_CODE boost::system::windows_error::broken_pipe >>> #else >>> using boost::asio::posix::stream_descriptor; >>> typedef stream_descriptor platform_stream; >>> typedef int platform_descriptor; >>> # define PIPE_EOF_ERROR_CODE boost::asio::error::eof >>> #endif >>> >>> boost::asio::io_service io_service_; >>> platform_stream pipe_; >>> std::vector<char> data_; >>> bool done_; >>> bool error_; >>> size_t total_size_; >>> boost::asio::strand strand_; >>> boost::thread thread_; >>> >>> PipeSession(platform_descriptor fd) : >>> io_service_(), pipe_(io_service_, fd), >>> { >>> } >>> >>> bool PipeSession ::start() >>> { >>> #ifdef LINUX >>> for (;;) >>> { >>> boost::asio::posix::descriptor_base::bytes_readable command(true); >>> pipe_.io_control(command); >>> std::size_t bytes_readable = command.get(); >>> >>> if(bytes_readable) { >>> break; >>> } >>> } >>> #endif >>> _thread = boost::thread(boost::bind(&PipeSession::run)) >>> >>> return true; >>> } >>> >>> void PipeSession ::run() { >>> try { >>> pipe_.async_read_some(boost::asio::buffer(data_), >>> strand_.wrap(boost::bind(&PipeSession::handle_read, >>> this, boost::asio::placeholders::error, >>> boost::asio::placeholders::bytes_transferred))); >>> >>> std::size_t num = io_service_.run(); >>> >>> } catch(const boost::system::system_error& e) { >>> done_ = true; >>> if(e.code() != PIPE_EOF_ERROR_CODE) >>> throw; >>> } >>> >>> static platform_descriptor CreatePipeFD(const std::string& pipeName, >>>bool readFlag) { >>> #ifdef WIN32 >>> HANDLE fd = CreateNamedPipe( >>> pipeName.c_str(), // pipe name >>> PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED, // read/write >>>access TODO PIPE_ACCESS_INBOUND? PIPE_ACCESS_OUTBOUND? >>> PIPE_TYPE_BYTE | // byte type pipe >>> PIPE_READMODE_BYTE | // byte-read mode >>> PIPE_WAIT, // blocking mode >>> PIPE_UNLIMITED_INSTANCES, // max. instances >>> BUFSIZE, // output buffer size >>> BUFSIZE, // input buffer size >>> 0, // client time-out >>> NULL); // default security attribute >>> >>> if(fd == INVALID_HANDLE_VALUE) { >>> throw std::runtime_error("CreateNamedPipe failed"); >>> } >>> >>> OVERLAPPED overlapped = {0}; >>> overlapped.hEvent = CreateEvent(0,TRUE,FALSE,0); >>> if(ConnectNamedPipe(fd, &overlapped) != FALSE || GetLastError() != >>>ERROR_IO_PENDING) { >>> CloseHandle(overlapped.hEvent); >>> CloseHandle(fd); >>> throw std::runtime_error("ConnectNamedPipe failed"); >>> } >>> #else >>> if(mkfifo(pipeName .c_str(), 0660) < 0) { >>> throw std::runtime_error("fifo failed"); >>> } >>> >>> int fd = open(pipeName c_str(), (readFlag ? O_RDONLY : O_RDWR) | >>>O_NONBLOCK); >>> if(fd <= 0) { >>> throw std::runtime_error("open failed"); >>> } >>> #endif >>> >>> #ifdef WIN32 >>> DWORD waitRes; >>> if((waitRes = WaitForSingleObject(overlapped.hEvent, LAUNCH_TIMEOUT >>>* 1000)) != WAIT_OBJECT_0) { >>> CloseHandle(overlapped.hEvent); >>> CloseHandle(fd); >>> throw std::runtime_error("WaitForSingleObject failed"); >>> } >>> CloseHandle(overlapped.hEvent); >>> >>> #endif >>> >>> return fd; >>> } >>> } >>> >>> -----Original Message----- >>> From: Peace [mailto:[email protected]] >>> Sent: Thursday, December 01, 2011 3:24 PM >>> To: [email protected] >>> Subject: Pipe transport for Windows >>> >>> Hello group, >>> >>> I am implementing Named and Anonymous Pipes transport for Thrift on the >>> Windows platform. The motivation for this is to provide a lightweight >>> local IPC transport for applications that run entirely on one system. Unix >>> already has domain sockets support in the TSocket transport but that does >>> not work on Windows. I would have preferred a cross-platform solution but >>> Windows pipes are much too different from the Unix implementations. What >>> are your thoughts on submitting this for possible inclusion? Would a >>> Windows-only transport bother people? Is there a better way to accomplish >>> this? >>> >>> >>> Regards, >>> Peace
