Add socket injection into client #147
Conversation
|
|
There was a problem hiding this comment.
I like the idea and motivation is quite clear. But current implementation has problems with re-connecting. IMO to address this, it would make sense to have some sort of SocketFactory in addition to SocketBase, please consider the following interface:
class SocketFactory
{
virtual ~SocketFactory();
virtual std::unique_ptr<SocketBase> connect() = 0;
};
And two implementors of this interface:
NonSecureSocketFactory and TLSSocketFactory, which handle everything related to establishing connection.
|
I thought about that, but another problem arises - re-connection mechanism uses blocking this_thread::sleep_for, that kinda defies purpose of this PR. There might be a workaroung in the lines of What you propose definitely makes sense, but for now would require |
|
Oh, may be i misunderstood you and you propose to move whole connecting + reconnecting logic to those factories? @Enmk |
clickhouse/base/sslsocket.cpp
Outdated
| #if defined(WITH_OPENSSL) | ||
| std::unique_ptr<Socket> SSLSocketFactory::doConnect(const ClientOptions& opts, | ||
| const NetworkAddress& address) { | ||
| std::unique_ptr<SSLContext> ssl_context; |
There was a problem hiding this comment.
IMO it would make sense to store SSLContext in a SSLSocketFactory and create it only once: in constructor.
clickhouse/base/sslsocket.h
Outdated
| public: | ||
| ~SSLSocketFactory() override; | ||
|
|
||
| #if defined(WITH_OPENSSL) |
There was a problem hiding this comment.
The whole file is not event visible to the build-system, if WITH_OPENSSL is not defined. Please remove the #if defined/#endif braces.
| }; | ||
|
|
||
|
|
||
| class SocketFactory { |
There was a problem hiding this comment.
To simplify issue with reconnecting and timeouts (and to provide a framework for upcoming multiple-hosts #139 ). Please consider updating the interface:
class SocketFactory {
public:
virtual ~SocketFactory();
// TODO: move connection-related options to ConnectionOptions structure.
virtual std::unique_ptr<SocketBase> Connect(const NetworkAddress &, const ClientOptions &) = 0;
// reconnects to a given address, maybe with a timeout or with any special procedures.
virtual std::unique_ptr<SocketBase> ReConnect(const NetworkAddress &, const ClientOptions &) = 0;
};
There was a problem hiding this comment.
There's a problem with NetworkAddress - its constructor uses blocking getaddrinfo; one way of soling this is to make some base class for NetworkAddress, but i feel like this would complicate the interface too much (and we already have some Java-style abstract factories here)
Introducing ConnectionOptions seems like a good enhancement for me, but i believe it has to be done in #139, or at least on the receiving end of upcoming merge conflict. Since SocketFactory is a new feature, there shouldn't be a concern with backwards compatibility
There was a problem hiding this comment.
Looks better, but please consider removing SleepImpl - too complicated and error-prone.
The way the Client sleeps before reconnecting is an implementation detail of the reconnection procedure. Hence it should be moved into SockerFactory.
Also, some other modifications to the SocketFactory interface are required to simplify setting multiple hosts (see #139 ).
clickhouse/client.cpp
Outdated
| return std::make_unique<NonSecureSocketFactory>(); | ||
| } | ||
|
|
||
| std::unique_ptr<SleepImpl> GetDefaultSleepImpl() { |
There was a problem hiding this comment.
Please consider changing the interface to get rid of the SleepImpl
https://github.com/ClickHouse/clickhouse-cpp/pull/147/files#r799244776
There was a problem hiding this comment.
Based on this
IMO, sleeping in a specific manner is still an implementation detail bound to the underlying transport mechanism
i did move sleepFor into SocketFactory
Please note that you are not required to implement multiple host configuration, just bear it in mind. |
It is indeed the detail of reconnection procedure, however said procedure not only just creates a socket, but also initializes some streams over it, handshakes, sleeps, retries and what not (might do more logic as in #139). All of that could be moved to some other class (say,
What i am saying is that i'm not sure if Moving
Could you please elaborate on that, what exactly you find error-prone? And about #139 - maybe out conflicting logic could be resolved together with the actual merge (and there would be a merge conflict for sure)? |
|
@itrofimow please see answers inline
IMO, sleeping in a specific manner is still an implementation detail bound to the underlying transport mechanism, so separating those may bring more harm than good.
I think we should ban sleeping in any other piece of code.
One has to keep two things in-sync, paying close attention that
What's even worse, most of the time, any combination might seem to work (since sleeper is not used in the good-path). But may break in a weird way in some edge case. And networking code is hard to test for edge cases. |
|
@Enmk please have a look at my inline answers One could argue that sleeping in a specific manner isn't necessary bound to the underlying transports async nature, and that it's the caller who is responsible for keeping provided |
|
@Enmk kindly re requesting review in case this PR got lost a bit |
These changes unveil the potential of the library for asynchronous applications (namely, web-servers) where blocking IO is not desirable.
The ability to inject some async-based communication channel should also ease creation of bindings for languages/technologies that strive for c++ performance but cant allow blocking IO (python, Node.js, etc..)
Submitted on behalf of a third-party: Yandex N.V.