Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stored procedure to produce push-messages never breaks on client disconnect #3859

Closed
cyanide-burnout opened this issue Dec 6, 2018 · 0 comments
Assignees
Labels
bug Something isn't working iproto
Milestone

Comments

@cyanide-burnout
Copy link

cyanide-burnout commented Dec 6, 2018

Tarantool version:
1.10

OS version:
Debian 8 x64, Debian 9 x64

Bug description:
Following stored procedure never stops when client disconnects
function registerPushChannel(network) local channel = channels[network] if channel == nil then channel = fiber.channel() channels[network] = channel end while box.session.id() ~= 0 do local data = channel:get(2) if data ~= nil then box.session.push(data, 1024) end end end

Steps to reproduce:

Optional (but very desirable):

Konstantin Osipov admin, [6 Dec 2018 at 13:20:16]:
push не бросает исключений

почему у вас box.session.id() не умирает - тоже поянтно. он умирает после того как disconnect trigger отработал

а disconnect trigger запускается после того как последние пуши отрабатывают

по хорошему надо проверять возвращаемое значение пуша

но ен факт что пуш вообще замечает отваливашегося клиента

судя по всему вы наступили на багос, а точнее на аспект поведения который мы не продумали.

@Gerold103 Gerold103 self-assigned this Dec 6, 2018
@Gerold103 Gerold103 added bug Something isn't working iproto labels Dec 6, 2018
@kyukhin kyukhin added this to the 1.10.3 milestone Dec 7, 2018
Gerold103 added a commit that referenced this issue Dec 11, 2018
Disconnect cmsg is a message that is used by iproto
thread to notify tx thread that a connection is dead
and has no outstanding requests. That is its
tx-related resourses are freed and the connection is
deleted.

But the text above is clear definition of destroy. The
patch harmonizes cmsg name and its puprose.

Secondly, the patch is motivated by #3859 which
requires separate disconnect and destroy phases.

Needed for #3859
Gerold103 added a commit that referenced this issue Dec 11, 2018
Before the patch on_disconnect triggers were called
only after last outstanding request had finished. It
was enough for most goals. But after box.session.push
was implemented, it appeared that it is impossible to
return an error from push() if a connection is closed.

Tx session just does not know that a connection is
already closed. The patch splits destroy and
disconnect phases of an iproto connection lifecycle.
Disconnect calls on_disconnect triggers and resets
iproto socket fd. Destroy frees resources.

Needed for #3859
Gerold103 added a commit that referenced this issue Dec 11, 2018
Disconnect cmsg is a message that is used by iproto
thread to notify tx thread that a connection is dead
and has no outstanding requests. That is its
tx-related resourses are freed and the connection is
deleted.

But the text above is clear definition of destroy. The
patch harmonizes cmsg name and its puprose.

Secondly, the patch is motivated by #3859 which
requires separate disconnect and destroy phases.

Needed for #3859
Gerold103 added a commit that referenced this issue Dec 11, 2018
Before the patch on_disconnect triggers were called
only after last outstanding request had finished. It
was enough for most goals. But after box.session.push
was implemented, it appeared that it is impossible to
return an error from push() if a connection is closed.

Tx session just does not know that a connection is
already closed. The patch splits destroy and
disconnect phases of an iproto connection lifecycle.
Disconnect calls on_disconnect triggers and resets
iproto socket fd. Destroy frees resources.

Needed for #3859
Gerold103 added a commit that referenced this issue Dec 21, 2018
Before the patch vtabs were stored in a global array
only, called registry and accessed by session.type to
call a virtual method. But it does not allow to change
session vtab without affecting session type. This
feature is necessary to be able to outdate binary
sessions whose socket is closed.

Outdated, closed, sessions will return errors from all
its methods.

Needed for #3859
Gerold103 added a commit that referenced this issue Dec 21, 2018
Once a connection is closed, a long-running user
request can not learn about this occasion. Even
box.sesion.push() still works.

This patch makes such disconnected session 'rotten'.
So a user can determine if a connection is closed by
looking at session.fd() == -1, or checking for
errors from box.session.push().

Closes #3859
locker pushed a commit that referenced this issue Dec 24, 2018
Before the patch vtabs were stored in a global array
only, called registry and accessed by session.type to
call a virtual method. But it does not allow to change
session vtab without affecting session type. This
feature is necessary to be able to outdate binary
sessions whose socket is closed.

Outdated, closed, sessions will return errors from all
its methods.

Needed for #3859
@locker locker closed this as completed in 43af2de Dec 24, 2018
locker pushed a commit that referenced this issue Dec 24, 2018
Before the patch vtabs were stored in a global array
only, called registry and accessed by session.type to
call a virtual method. But it does not allow to change
session vtab without affecting session type. This
feature is necessary to be able to outdate binary
sessions whose socket is closed.

Outdated, closed, sessions will return errors from all
its methods.

Needed for #3859

(cherry picked from commit 69d3678)
locker pushed a commit that referenced this issue Dec 24, 2018
Once a connection is closed, a long-running user
request can not learn about this occasion. Even
box.sesion.push() still works.

This patch makes such disconnected session 'rotten'.
So a user can determine if a connection is closed by
looking at session.fd() == -1, or checking for
errors from box.session.push().

Closes #3859

(cherry picked from commit 43af2de)
gdrbyKo1 pushed a commit to gdrbyKo1/tarantool that referenced this issue Jan 17, 2019
Before the patch vtabs were stored in a global array
only, called registry and accessed by session.type to
call a virtual method. But it does not allow to change
session vtab without affecting session type. This
feature is necessary to be able to outdate binary
sessions whose socket is closed.

Outdated, closed, sessions will return errors from all
its methods.

Needed for tarantool#3859
gdrbyKo1 pushed a commit to gdrbyKo1/tarantool that referenced this issue Jan 17, 2019
Once a connection is closed, a long-running user
request can not learn about this occasion. Even
box.sesion.push() still works.

This patch makes such disconnected session 'rotten'.
So a user can determine if a connection is closed by
looking at session.fd() == -1, or checking for
errors from box.session.push().

Closes tarantool#3859
@kyukhin kyukhin added the tmp label Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working iproto
Projects
None yet
Development

No branches or pull requests

3 participants