Added support for C++ Nonblocking SSL Server

[dthaluru]

No description provided.

[jeking3]

The support for SSL vs. non-SSL should be opaque to the server implementation. That is a transport concern. If you look at the other servers, they know nothing about SSL/TLS at all. Remember that a TSSLSocket "is a" TSocket, and a TSSLSocketServer "is a" TSocketServer. TNonblockingServer already has a constructor that can take a transport factory. It looks like it is lacking a constructor that allows you to pass in a SSL server transport. All the constructors seem to take a port, and the class uses regular sockets. The class should be refactored to have additional constructors that take a serverTransport in place of a port number. Ideally, we would deprecate the port number constructors in favor of serverTransport constructors, because it should not be up to the TServer to create the server transport.

I would recommend that you look at the TServerFramework set of constructors on which the other servers are now based. The simple / threaded / threadpool servers provide a good example on how to provide non-SSL or SSL services by passing in different transports. You can see the example implemented in the TestServer.cpp file in test/cpp. If TNonblockingServer is lacking a constructor that would allow you to pass in the server transport, that should be added. One of the constructors in TServerFramework allows the caller to designate the serverTransport and the I/O transport factory.

To make a TNonblockingServer that uses SSL, one should construct TSSLSocketFactory and pass it in as the inputTransportFactory / outputTransportFactory; and from the factory a TSSLServerSocket can be created and pass it in as the serverTransport.

So a constructor that looks like this would allow TNonblockingServer to handle SSL/TLS:

  TNonblockingServer(const boost::shared_ptr<TProcessor>& processor,
                     const boost::shared_ptr<TServerTransport>& serverTransport,
                     const boost::shared_ptr<TTransportFactory>& inputTransportFactory,
                     const boost::shared_ptr<TTransportFactory>& outputTransportFactory,
                     const boost::shared_ptr<TProtocolFactory>& inputProtocolFactory,
                     const boost::shared_ptr<TProtocolFactory>& outputProtocolFactory,
                     const boost::shared_ptr<ThreadManager>& threadManager
                     = boost::shared_ptr<ThreadManager>())

We definitely should not need to duplicate the entire server class in order to change the transport.

Also the change to TSocket is a breaking change - is it really necessary?

[jeking3]

Is there an Apache Jira ticket for this enhancement?

[dthaluru]

@jeking3 I have addressed all issues and updated the PR. I am not aware of any Apache Jira ticket for this.

[jeking3]

Okay, this is an improvement over the previous PR. The addition of a test is really good to see - I like to see that! Any cross (make cross) tests that were previously disabled for nonblocking + SSL should be re-enabled for C++, if that combination is actually tested by crosstest (I have not checked).

I would suggest that if you can find a way to implement this without having TNonblockingServer know anything about SSL vs non-SSL... it would be ideal. None of the other servers have any idea about whether they are using SSL or regular sockets, or unix sockets, or pipes, or file mailboxes. If we can get TNonblockingServer to accept the base server socket and socket factory classes then it will be compatible with all transports which will make it more useful.

[dthaluru]

@jeking3 I have added Transport layer for Nonblocking server as suggested. And I have also added a test for TNonblockingSSLServer.

[jeking3]

Thanks! I am going to need some time to review it further. The server layer should know nothing about the transport which it looks like you addressed, but similarly the transport layer should know nothing about the server. There's a new TNonblockingSSLServerSocket class here - this seems to be combining the responsibility of the transport with that of knowing the server type is.

Here are a couple things to consider from a quick review:

1. I wonder if you need the Nonblocking socket classes; instead if you have TNonblockingServer call TSSLSocket::setLibeventSafe(false) after asking for the socket from the factory - you might not need a Nonblocking factory at all (a nonblocking socket factory combines server and transport concerns).

2. Is there any reason isLibeventSafe() branches only apply to TSSLSocket? If the same reasoning applies to TSocket as well, then perhaps that logic moves into TSocket so TNonblockingServer uses TSocket the same way as TSSLSocket?

I'm curious what the specific "not libevent safe" mechanisms are. I'll pick that up from further code review.

[dthaluru]

One of the main reason I had separate transport for Nonblocking is, blocking has a feature called interruptible which does not really apply for nonblocking. And additional api's are added for Nonblockingtransport which are not required in blockingtransport.

And currently TSSLSocket underlying implementation is Nonblocking by default and uses select api.
TNonblockingServer implementation uses libevent which inturn calls select api's. We cannot use libevent with select as there will be a race condition between select and libevent as both poll for events on registered sockets. Ideally there should be another class called TNonblockingSSLSocket which can be used with NonblockingServers. TSocket underlying implentation does not use select, so there is no need to have any libeventsafe methods.

[jeking3]

Interruptable sockets can be controlled by the server during creation, I believe - one should not need another class to use TSocket in a non-interruptable fashion. You may be right about the select() issue, as the TNonblockingServer really had its own socket implementation it was using. I'll take a look at the other changes shortly.