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

box.session.push rework #6107

Closed
Gerold103 opened this issue May 30, 2021 · 6 comments
Closed

box.session.push rework #6107

Gerold103 opened this issue May 30, 2021 · 6 comments
Assignees
Labels
app feature A new functionality

Comments

@Gerold103
Copy link
Collaborator

Gerold103 commented May 30, 2021

There is a lot of feedback that box.session.push() is broken and not usable. Not many details though. This ticket attempts to gather the issues in one place.

Push requires a fiber on the storage side
Push can be done only from inside of a request. Request always uses a fiber and a message object in iproto thread. This means a subscription becomes quite expensive: it wastes fiber which takes at least kilobytes of memory for its stack and it wastes a unit from net_msg_max.

It would be probably much easier to use if the pushes could be done for any session not necessary inside of a request.

Netbox is_async and pushes are not very compatible
is_async does not work with on_push callback. It can only work with future:pairs(). Which means it is not possible to handle the pushes in a fully async way. Either the request itself is blocking and supports on_push, or it is not blocking but the pushes can only be consumed in the end.

Second part of the async problems is that I can't detect when the subscription ends. For example, here is a function implementing subscription:

local function storage_subscribe(sync)
    local session_fd = box.session.fd
    local ok, err = subscribe_register(sync)
    if not ok then
        return nil, err
    end
    while true do
        if session_fd() < 0 then
            return
        end
        // ...
    end
end

When I couldn't register it, I want to return an error and end the sub using return nil, err. But on the client side I won't receive anything until call future:wait_result() or unless I poll future:result(). Both options are not very async.

I need a way to implement a fully async sub. Without having to care about the future object at all. And to be able to notice when the sub ends.

Back-pressure
Using box.session.push for the same session multiple times has no back-pressure control. A one can call push hundreds of times on the same session without waiting for the data to be sent. It might result into high memory usage and even OOM if there are many events.
Would be good to be able either to be able to wait for the pushes flush somehow, or implement one-shot subscription. When you push something into a session and close the subscription. The client will re-subscribe when receives the previous event.
The latter looks interesting. It can be done even without box.session.push using normal requests. But to make them async netbox needs to be able to consume response of a request asynchronously. is_async is cool, but it still requires to call wait_result() in the end. It can't invoke a callback instead.
One-shot sub could work if is_async also would allow to specify a callback and never wait for the response in a blocking way. Re-sub would happen from the callback if needed.
Although if it is not box.session.push(), it will keep one fiber on the storage waiting for an event.

This lead to a bigger question. Can we actually make possible to send a response on a request asynchronously on the server side? Without blocking one fiber for that?

@rosik
Copy link
Contributor

rosik commented Jul 6, 2021

I think I found a way to marry is_async with on_push. It's ugly, but it works.

Client Server
box.cfg({listen = 3301})
box.schema.user.grant('guest', 'super')
function sub()
    ch = require('fiber').channel()
    while true do
        x = ch:get()
        if x == nil then
            return "Stream finished"
        else
            box.session.push(x)
        end
    end
end
c = require('net.box').connect('3301')
f = c:call('sub', nil, {is_async = true})
f.on_push = print
ch:put("foobar")
table: 0x407451f0    foobar

table here is f.on_push_ctx

@Gerold103
Copy link
Collaborator Author

The problem is that in case you can't call sub due to, for instance, lack of access rights, you won't get any response. You need to call f:wait_result() somewhere. And this is one of the main problems.

@rosik
Copy link
Contributor

rosik commented Jul 6, 2021

It's not a problem of pushes itself, but of notifications implementation.

Imagine, a client sends a netbox request and surely knows it won't get any response until there's some event on the server. How could it tell network problems from the absence of an event? It can't happen without some additional conventions. For example, the server may always push approval at the beginning of the sub function. And, accordingly, the client may wait for the first push synchronously.

In other words, we can speak of it as of a netbox push-based sub-protocol and start designing it. Such an inspiring opportunity, isn't it? :-)

@Gerold103
Copy link
Collaborator Author

It's not a problem of pushes itself, but of notifications implementation.

Exactly. And the ticket is about problems of box.session.push() as API, not of the binary protocol.

Imagine, a client sends a netbox request and surely knows it won't get any response until there's some event on the server. How could it tell network problems from the absence of an event?

Doesn't it prove my point? With the example you created you won't get any error. Regardless of its type.

And, accordingly, the client may wait for the first push synchronously

That means you need a fiber to wait for the first push synchronously. I wanted to have 0 additional fibers for the subscriptions, at least on the client. Besides, it won't help if after the first push the network gets broken or the instance dies - the connection breaks then and you won't notice it. Because you don't have any fiber doing future:wait_result() on the async request. If you can spare some fibers for that, all is working even now - span a fiber-per-subscription on the client and do blocking requests.

In other words, we can speak of it as of a netbox push-based sub-protocol and start designing it. Such an inspiring opportunity, isn't it? :-)

Probably I misunderstood something? It is not a sub-protocol. It is only a matter of the API. Both on the storage and on the client (netbox). On the netbox the final goal is to never block and never create additional fibers.

@sergos
Copy link
Contributor

sergos commented Oct 11, 2021

similar to #6257

@kyukhin kyukhin modified the milestones: 2.10.0-rc1, 2.10.1 Dec 30, 2021
@kyukhin kyukhin modified the milestones: 2.10.0, 2.11.0 Jun 1, 2022
@kyukhin kyukhin removed this from the 2.11.0 milestone Aug 25, 2022
@sergos sergos removed the teamS label Oct 18, 2022
@Mons
Copy link
Contributor

Mons commented May 30, 2023

Decided to get rid of box.session.push in 3.0+

@Mons Mons closed this as not planned Won't fix, can't repro, duplicate, stale May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app feature A new functionality
Projects
None yet
Development

No branches or pull requests

6 participants