[WIP] ARROW-4418 [Plasma] replace event loop with boost::asio for plasma store

[suquark]

reopen this PR. See #3704 for the older one.

[suquark]

Let me try to address all these conflicts first

[github-actions]

https://issues.apache.org/jira/browse/ARROW-4418

[suquark]

Just experienced a terrible rebase, something may have messed up. I'll try to fix them.

[mehrdadn]

Hi @suquark! I've been looking at using Arrow on Windows, but I noticed in your PR there are some Windows incompatibilities. For asio::local, would it be possible to just use TCP sockets on Windows now? If it turns out there are performance/other such issues then I'm happy to help write a wrapper for named pipes (though it would take some time). And regarding any other bits, let me know if you have questions on how to do things on Windows, I have a fair bit of experience there so I can likely help you figure it out. :)

[suquark]

hi @mehrdadn thanks for you comment! yep, one purpose of this PR is to prepare for multi-platform support. asio::local doesn't seem to be helpful for windows, but I will just keep this for this PR, because this PR affects a bunch of files and suffers from conflicts (nearly) weekly, and the review progress is quite slow. I decide to get it merged as soon as possible once I have time. I (or you) can create a following PR to deal with windows issues, since this PR has removed most posix-specified dependencies.

[fsaintjacques]

I'm going to close this for now, you can re-open once it is ready for review.

[mehrdadn]

@suquark Hey Ryans, I was wondering what the state of this PR is. Do you recall what the "best" commit was, and whether it was ready to be merged? Were there any blocking issues (aside from rebase + code-review)? I'm considering taking a look and seeing if I can rebase it (and hopefully add Windows support for), but I'm not sure what commit is the best one to look at or whether there were any pending issues I should keep in mind, so if you could let me know that'd be great. Thanks!

[suquark]

@mehrdadn The major issue is that the PR involves too many files, and it keeps conflicting with the master branch (the plasma API changes heavily over time). After I kept it untouched for a few months, I failed to rebase it on master anymore, because too much conflicts happen across too much commits. So later I tried to squash and skip some commits while rebasing it on master, but unfortunately something seems broken after the rebase. It takes time to figure out which part is wrong, because it is a bit messy now. I think I should be still responsible for cleaning up the messy stuff, otherwise it would be too hard for other developers to understand the messy part. Let me see if I can divide it into smaller PRs.

The first part is https://issues.apache.org/jira/browse/ARROW-8030
The PR is here: #6559

[mehrdadn]

Ah I see, that sounds like a lot of work. If it's only intended for Windows compatibility, do you feel it's worth the cost? I'm unclear on what's involved, but I (naively, I think) feel like merely communicating an IP address and switching to AF_INET should be sufficient to establish communication on Windows, and it might not need Asio.

To give you some idea of possible issues, I'll think aloud here; I don't really know what the right approach is.

One thing that's complicated (regardless of whether we use Asio) is that there's a fundamental difference between UNIX sockets and TCP sockets, and that is the fact that pre-allocating TCP/IP sockets is not a great idea: if we allocate a port and tell Plasma to use that port, we have to free the port first (so Plasma can bind), which poses an inherent race condition because someone else might allocate it in the meantime. Which I guess could be an issue with UNIX-domain sockets too, except names can be selected to avoid accidental collision. This means that, to use TCP sockets, we'd ideally let Plasma allocate its own port, and then tell us what that port number is somehow.

This is also somewhat incorrect too, because it's possible to duplicate sockets on Windows across processes, so presumably we could allocate the socket and pass it to Plasma. But doing so would not be straightforward because it requires the owner of the socket to know the PID of the target process (WSADuplicateSocket) and to pass a giant data structure to the target (WSA_PROTOCOL_INFO). So it's not merely a question of passing an address on the command-line or something like that.

Another solution is to use Windows named pipes, which are very similar to UNIX-domain sockets as far as naming goes. The thing is though, they don't interoperate with socket APIs, and Boost.Asio kinda has its own separate abstraction over them, and I have yet to find a common abstraction between those classes and the socket classes (though maybe there is something and I just haven't found it). So this would probably be nontrivial too.

I can also think of more complicated solutions, like using named pipes for initial discovery, TCP for subsequent communication. So this is another approach, and it may be easier?

Anyway, these are just the conceptual issues I know. There are also some other considerations I'm probably not familiar with. I do see one such issue might be the fact that duplicating handles (e.g. RecvFd) on Windows requires the sender to know about the receiver (or vice-versa) so one of them can call DuplicateHandle when the other is guaranteed to be alive. I think I actually do have some code lying around for this (even for doing this over a socket), although it's inefficient (it needs to discover the PID of a socket peer, which is slow) and it also has a race condition (although I don't recall if it is a big deal in practice), but it might have some roadbumps along the way.

Overall, I do think some of these would indeed likely benefit from restructuring of the code around Boost.Asio, but I'm much less clear on how much they would benefit. If the entire goal of the PR was Windows compatibility for Plasma, I'm not sure if it would reduce the work for that to (say) 20% of what it would be otherwise, or just (say) 80%; for all I know, it could be either of those.

So if that's the case, since you're much more familiar with Arrow, my suggestion would be this: before putting in a massive amount of effort into this, it might not be a bad idea to consider how difficult it would be to make TCP sockets work (on Linux), since if they do, they can be ported rather directly to Windows. If you feel it's significantly easier (at least conceptually), maybe we should consider doing that instead; I can try to give it a shot and let you know if I run into trouble. But if you feel it might be difficult, then maybe getting this PR to work is better.

[suquark]

@mehrdadn Thanks for your reply! Yep, your concerns totally make sense. This PR is not a direct path for windows support, but it does try to address some issues about windows support:

1. Current implementation uses a low-level eventloop library, so a lot of unix-only headers are used to fulfill all functions required by Plasma.
2. Raw linux file descriptors are used everywhere. As far as I know, windows socket functions return types like SOCKET other than int.

By using asio we can get rid of these issues pretty easy. This PR was meant to move fast so that we can implement windows support after it is merged. Do you think these two issues seems significant for you? If they are, then it should still be a good idea to first merge this PR.

However, as you mentioned, this PR is not a solution for:

1. How to duplicate shared memory handles between processes given a connection. It is related to the IPC mechanism, once we have decided which IPC (tcp/namedpipe) to use, the solution would be very clear. We only need to use the proper protocol for the target operation system.
2. The actual design of windows IPC. This is indeed worth discussing. Using TCP is a bad idea if we just need a version that is testable under windows. The earlier version of plasma store has an object manager interface that uses TCP, and hardly any users complains about the port issue. Maybe I am wrong about the fact, a lot of cross-platform applications just use TCP under windows, while they have unix domain socket support under linux (for example, redis). I don't understand why they don't prefer namedpipe. If named pipe is proven to be stable and commonly used for local services, I would prefer it over TCP; otherwise we could just use TCP as other open source applications.

[mehrdadn]

Regarding UNIX-only headers, it can be anything from trivial to impossible depending on which APIs we need. I actually already have a lot of shims for some headers in Ray (see here), but they're fundamentally incomplete. For example, I have sys/socket.h (which isn't on Windows), but I don't think I can have sys/types.h because that would conflict with the existing one in Windows (which is similar but different). But that isn't an issue anyway, since I could just declare what we need in another header and include that instead.

So the question comes down to what APIs we need from those headers. For basic file descriptor I/O like read()/write(), they already exist in the Windows C runtimes, so there's no issue. For more advanced stuff like fcntl(), it may pose an issue, but in particular cases it's easy enough to rewrite (e.g. via ioctlsocket()) which I've already done in Ray. For socket APIs, it's less a question of the headers, and moreso a question of (a) passing HANDLEs vs. FDs, (b) opening in overlapped vs. synchronous mode (which can affect the ability to open in blocking vs. non-blocking modes and reading/writing accordingly), and (c) using epoll() and such.

For (b), it can be worked around (I just did this for Redis by using WSARecv instead of recv or read, to read synchronously from an overlapped socket), and for (c), as long as select() or poll() can be used (instead of epoll or kqueue), we can at least have a low-performance solution on Windows without much difficulty. (In fact I have something for that implemented here, but I haven't been able to test it yet.)

For (2), it's a nuisance to deal with int file descriptors, but whether or not it's difficult depends on the usage. The Windows C runtimes support int FDs as an indirection over HANDLE, so we can keep passing around sockets as ints; that part isn't a big deal. The socket APIs require SOCKET (aka which is a HANDLE), but we can use _get_osfhandle() and _open_osfhandle() to translate between them at the call sites as well. The most annoying aspects are closing the sockets (which requires remembering to use closesocket rather than close or CloseHandle!) and the fact that we have to be careful not to use read()/write() if we use overlapped sockets. I've already had to solve these for hiredis the past few days, so if the situation here is similar, it shouldn't be too bad. But I haven't been able to test the code for that much either, so it's also possible there are bugs I'm unaware of that require more effort to solve.

For TCP, the port race condition isn't Windows-specific; it'd occur on any OS. But I think you're right, it might not be an issue, because in practice you can just ask the server to start on a random port, and if it's not allocated, just keep re-launching it with more random ports until it finds one (which is what Ray seems to do). It's not an elegant and one-shot like what I was imagining, but in practice it's probably good enough, at least as there's a way to verify the server has succeeded in allocating the port the client is launched.

As for named pipes, well, named pipes are most definitely stable; they're a very core part of Windows and are used in many places outside it as well (e.g. see Chromium), and they're superior to sockets at least performance-wise if not in other ways. It's just that they're non-portable and only support local communication, so people don't bother supporting them if they don't feel they need to (maybe their bottleneck is elsewhere), or if they feel they might need remote communication over the internet.

Oh, there's also one more issue, which is that you'd want to check WSAGetLastError() instead of errno on Windows; I had to deal with this for hiredis as well. This is probably more tedious to address here, because I doubt I can just replace every usage blindly like I could in hiredis. But it's still a matter of just replacing text in some places, not changing the design.

[jikunshang]

Hi Ryans, I cherry pick your PR and did some work based on it. I appreciate it a lot :)

I know you got some trouble in resolve conflict, and I'm happy to help you with that if you don't mind. Feel free to let me know if you need help.

[suquark]

I create another PR that makes rebase easier: #6579

[suquark]

@jikunshang Thanks for your comments! Currently I am thinking about how to divide this huge PR into smaller ones, because even after we work out how to address all conflicts, the review process would be slow. It could be helpful if you have any ideas. At least for next several PR I am going to perform some code cleaning first.

[jikunshang]

@suquark Thanks for your reply. I don't have idea about how to break down yet, since import boost::asio would have about 1000+ lines code...

[suquark]

Currently I am waiting for the merge of #6587

[mehrdadn]

No rush on this; I think I have workarounds for Arrow as far as Windows goes!