[tarantool-patches] Re: [PATCH 1/3] netbox: allow to create a netbox connection from existing socket

  • From: Konstantin Osipov <kostja@xxxxxxxxxxxxx>
  • To: "v.shpilevoy@xxxxxxxxxxxxx" <v.shpilevoy@xxxxxxxxxxxxx>
  • Date: Tue, 27 Mar 2018 00:55:49 +0300

* v.shpilevoy@xxxxxxxxxxxxx <v.shpilevoy@xxxxxxxxxxxxx> [18/03/22 22:54]:

22 марта 2018 г., в 22:33, Konstantin Osipov <kostja@xxxxxxxxxxxxx> 
написал(а):

* Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx 
<mailto:v.shpilevoy@xxxxxxxxxxxxx>> [18/03/22 22:18]:

The patch generally looks good to me, I can only nitpick on new
things. 

How about renaming do_connect to connect_impl and wrap_connect to
wrap_socket or simply wrap, or maybe bless or imbue?

Ok.


+        if existing_connection then
+            connection = existing_connection
+            existing_connection = nil

Why is it necessary to set it to nil?

On reconnect it will call protocol_sm again, and it must no get
old existing_connection again.

This code is a mess and you make it even more messy.

What frustrates me most is that it's a trivial refactoring and
you prefer explaining why a fix is impossible rather than making
a good fix.

You *can* change the responsibilities of all of these functions so
that everything works nicely without extra branches and having to
pass existing_connection through several layers of calls.



+            assert(greeting)
+            assert(greeting.protocol ~= 'Lua console')

Please keep in mind that in Lua there are no asserts - it's a
runtime check.
kk
Ok.

+
+local function wrap_socket(connection, greeting, url, opts)
+    if connection == nil or type(greeting) ~= 'table' then
+        error('Usage: netbox.wrap_socket(socket, greeting, [opts])')
+    end
+    if greeting.protocol == 'Lua console' then
+        error('Can not wrap console socket')
+    end
+    opts = opts or {}
+    if not opts.user and not opts.password then
+        opts.user, opts.password = url.login, url.password
+    end
+    return do_connect(url.host, url.service, opts, connection, greeting)
+end

Looks like you should make the next step, i.e. take the piece of
do_connect which establishes a connection and move it to
connect(), and then you can the remains of do_connect into wrap()
and call wrap() connect().

Did you try to do it? Had any trouble?

Connection establishment must be in connect inside
create_transport, because even if an existing socket was
wrapped, it still can have opts.reconnect_after option and the
following scenario:
socket = ...
con = netbox.wrap_socket(socket, ..., {reconnect_after = ...})
--
-- use connection ...
--

Suppose it is broken due to error. Then netbox will try to
establish a new connection after 'reconnect_after' seconds.

You can leave reconnect in protocol_sm, but you can make sure
protocol_sm is never called without an existing connection.
You can make the first connect directly in create_transport(),
or better yet, as I first suggested, outside, removing the need
for connect_impl().


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov

Other related posts: