-
-
Notifications
You must be signed in to change notification settings - Fork 728
Project ACID (Automatic Client-ID) aka "software rugpull" #4552
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for another impressive pull request, @evnchn!
To summarize: Even though this PR might be an improvement over the "old" rugpull, it still adds quite some complexity that needs to be balanced with the expected benefit. |
Regarding your points:
Additionally, I shall add
*Issue will arise when, say, the page is configured to be one-off visitable which does some action which cannot be repeated # assume some middleware already ensures that /visit_once can be visited once and once only throughout the lifespan of the server...
@ui.page("/visit_once")
def page visit_once():
do_this_once_and_once_only()
ui.label("The action meant to be done once and once only has been done!") On the ACID maneuver, So, either:
|
Thinking about it, seems like having it as an opt-in characteristic Would say, ACID maneuver bypassing FastAPI middleware logic is fine, as long as the page code checks for the authorization, and return a Implementing our own authorization system (no longer rely on FastAPI middleware) would help with the adaptation process. Imagine @ui.page(authorization_function=my_auth_func) where Though, it would still be better if we can somehow pass the reconnection attempt as if it was a HTTP request through the FastAPI middleware again, but paying attention not to re-execute the entire page function (would defeat |
Ah, I'm finally realizing that you are not reconnecting without loosing any messages. You just short-circuit the HTTP request that would be needed to get a fresh page. The following code would restart from 0 (as if you would reload): @ui.page('/')
def index():
n = ui.number(value=0)
ui.timer(1.0, lambda: n.set_value(n.value + 1)) So you're enforcing a handshake right on disconnect and re-creating the client if it has already been pruned. The code duplication feels a bit fragile though, and the problem with lost parameters like |
"Ah, I'm finally realizing..."Yes. What you said in the last comment is true. You finally get the gist of the ACID approach. I like how even the library maintainer read it wrong the first time. Truly goes to how mind-bending this PR is (or maybe my communication skills is bad, which it is 😅). That said, we should be very careful as we proceed to communicate and navigate this PR. "Why don't you simply increase the
|
If you may feel that the ACID approach is still a bit sketchy, we may have to resort to marking this approach as a beta feature, and potentially excluding the use of this approach in the security policy (meaning there won't be any support on security, and if you use this new feature and got hacked, you're on your own) But let's try not to do that, and polish this PR a bit more first. More minds on the PR more better 🧠 |
All your listed benefits state that users do not want a browser reload. But why? And ACID is also doing a kind of a reload. So whats the benefit of not doing a standard reload? |
What I mean with "fragile" is that we now need to keep page.py and nicegui.py in sync when changing the code in the future. We can certainly refactor it to avoid code duplication. And this might help to clarify what this code is actually doing, namely creating a new client instance based on certain information. To summarize: I'm still trying to simplify the concept, or at least my understanding of it. But like @rodja asked: Why not doing a normal reload? Sure, after a short disconnect you might want to avoid a big(?) HTTP request. This is what |
I think your understanding of this new approach has been correct since the comment beginning in "Ah". The code refactoring sounds like a good idea, such that the logic for creating the client according to the "recipe" remains the same. Now to answer the big questions:
I may be thinking too many steps ahead here, but I still think there is value in getting this PR working, for servers with less-than-ideal connection quality, as well as pavig the way for future PRs. |
Same is true for ACID. You need to fully clear the page and load elements anew. Otherwise you can not be sure to have the same state between frontend and backend.
"clients from distance" are always a problem with NiceGUI (and other interactive frameworks). Each interaction needs to be send via websocket and so the whole page feels laggy if you have a roundtrip of more than say 60 ms. So I would argue that this is not a supported use case for NiceGUI in general.
But you need to do a full reload with ACID. I am sceptical about HTTP round trip beeing measurably slower than a socketio handshake (which is also happening via HTTP).
But after loading the whole cached content is replaced by the new content send via socketio. There is only some stuff in the header which can be cached. And that might very well be done better without ACID (for example by putting as much of it in a separate file).
The main benefit of #2811 is that it does only replace the parts on the page which changes. ACID always needs to evaluate and render everything anew. Sorry for pushing back so hard. But we need to think about NiceGUI's maintainability and ease of use. I still not see a clear benefit of ACID which would justify the hardship it brings with it. To conclude with something positive: there might be hiding something like "Backend-less NiceGUI" behind ACID. I think of a small (separate) project which uses NiceGUI but does not establish a socket communication at all -- and hence could be used for serving static, cachable pages written in Python. |
First of all, as the library maintainer, you can reject the PR if you think it is too tough to work on it. There is no need to say sorry. However, I still see some misconceptions, and I'd like to get the point fully across for your most accurate judgement. TL-DR 1: ACID maneuver is NOT fast! It could possibly be slower than a plain-old HTTP load! However, since we are not invoking the browser, the ACID maneuver can take as long as it needs, but we will not get flashing white screen, not even a millisecond. Have you checked out the code in this PR under adverse network conditions? You can set the network bandwidth and request latency in custom profiles in Network tab in DevTools as (reasonably) slow as you want, but there will still not be flashing any white screen when the ACID maneuver takes place. The content is just loaded in a snap 🫰, always. Not only that, you can try make the page content longer. The effect is more obvious then. I apologize for unable to screen-record on my other computer right now, but if you want, perhaps I can do a video demo? Misconceptions clarification
I believe not. Since for the ACID approach, the web page is not reloaded, but the script handshakes the server to obtain the latest content. In the following workflow of a NiceGUI page load:
Notably:
If you are comparing the round trip time between HTTP and a SocketIO handshake and come to the conclusion that the ACID approach is not worth it, then you have have misunderstood the point. Point is:
That would be right under current implementation, but note the following:
That would literally be the "super tiny, generic html page which establishes a websocket connection" @falkoschindler asked for in #1539 (comment)
Then, the page which is cached by Nginx / Cloudflare / etc, will have the same consistent but generic content (cache 99%), and the WebSocket message will just be a single update on the element
Note that:
Response to other points:
Backend-less NiceGUI would probably be a pain, because we would need to implement the reactivity in JavaScript (or use Brython?). However, humans just want immediate visual feedback when they click a button. They don't need to see data for that. That's why if you check #4451, you see the course of action for me to implement a card click button which navigates to another page, is not using JavaScript to clear the page and populate new content, but merely to make the button do I have thought of implementing such things ( Talking about the 3-way handshake taking an unexpectedly long duration, with the ACID approach, we can add extra animations and text like "Loading, please wait...". While, in a browser reload, all you will see as the browser does its thing is a blank white page which is unconfigurable as it is hardcoded into Chrome/Firefox/etc. |
"Backend-less NiceGUI" Hmm: I am now thinking either Then, such a page can be safely cached by proxies, because there is no connection to be made, and there won't be any concerns of missing Of course, this would mean no reactivity, or at least all reactivity is done via JavaScript on the browser side.... |
Brainstorming... So the thing with ACID is that we're creating a new client for the socket.io handshake, because the old one is deleted. But then, why delete the old client? Perhaps the client can be persistent, and shared, much like the auto-index page? So, while this PR is still in consideration, I'll explore ways to leverage the existing auto-index page for a more seamless reconnection experience. Meanwhile, I think, potentially, having a single shared client for the pages defined using @ui.page might be a good idea. That can be in another separate PR. |
Timeline wise, should we want to do this, it definitely must be 3.X release. Slipping such a big change in 2.X seems totally wrong. Even if this functionality can be totally disabled, expectations have to be adjusted with this PR, and it's a big feature, not your run-of-the-mill new UI element. |
Regarding the white flash: This seems to be a common problem (FOUC) and could be addressed by the way |
I think white flash (FOUC) is a solved problem. Because, the only styling that can show through is the blank page (recall all UI elements are shown via JS, so nothing's in there in the beginning), and the only styling (excluding crazy tricks such as Thus, it would suffice, if whoever sets the custom background color also includes the following in their code. Though, 2 ideas with regards to NiceGUI doing some of the heavy lifting:
But I'm not sure that's worth the pursuit. Seems like it would be quite a trouble and needs significant rework. Also, the benefit of manually inserting code would work for even older NiceGUI versions. |
This is a reimagination of the "software rugpull" PR #4487
TL-DR: Software rugpull is back, renamed to ACID (Automatic Client-ID). It enables zero-browser-reload (no F5) in server reboot, client disconnect for longer duration than reconnect_timeout, and content reload. Name is awesome, Code is clean, PR is clear.
Introduction to ACID (Automatic Client-ID) aka "software rugpull", for the uninitiated
The ACID approach (previously called "software rugpull") shakes up the foundation of NiceGUI that is "for a browser client, there must immediately be a matching client instance in the server, or the client must start over and challenges an almost unstated rule that, in tricky circumstances reloading is the best way to solve all issues
It is achieved by automatically generated a client instance in the server with a matching
client_id
as the one reported by the browser (which, either, was deleted due to timeout, never existed since the server had a reboot, or intentionally deleted forui.navigate.soft_reload
), then deceiving the handshake code below into always returning True (hence the "software rugpull" nature, ref: https://youtu.be/q5M0TwnkWUM?t=3735)What happened to the old PR
I'll be honest, that PR was one of the worse I have done.
What led to this new PR
Despite the above, the idea proposed in the old PR was sound, and this PR takes what was in that old PR, makes it better, and enables the 3 following functions:
reconnect_timeout
ui.navigate.soft_reload
, which reloads the content of the page without the equivalent of pressing F5 on the browser.And, to maximize the chance that this PR goes through, I have included:
Testing code
I know you can't wait 😉
Notable points:
Rough explanation of the commits
General modification to enable ACID maneuver
expose path of page on handshake + expose path of page to client for its handshake
This allows the browser client, when making a handshake to reconnect, to inform the server which page it was originally visiting (say
/async_page
). Subsequently in the ACID maneuver, the corresponding page function will then be found (for func, path in Client.page_routes.items():
) and swiftly executed so as to create the ACID client to match the handshake request.allow force-set ID for client
Since a client is matched via the
client_id
, and I personally don't want to implement logic to change theclient_id
in the browser side, I made it so that the server would accept whateverclient_id
was passed in to the server. This meant that simply doingself.id = str(uuid.uuid4())
would not be enough. Rather,self.id = force_id
is called ifforce_id
was passed inpage keep track of instances for ACID to find the right one
Makes sense shortly, since we need to re-find the page instance for
with Client(proper_page, request=None, force_id=data['client_id']) as fake_client:
The important bit
The crux of ACID (Automatic Client-ID) aka "software rugpull"
TL-DR:
I stand by my old comment in #4487 (comment)
Major difference being, in this new PR:
page
class with the same path. Use the correctpage
instance as provided by "page keep track of instances for ACID to find the right one"Client.connected
, and we can await the page function as usual. Since it is a mirror ofnicegui/page.py
, I have sneaked in changes from use background_tasks.create over bare asyncio.create_task #4551, which should be fine. If not we rollbacktab_id
andenviron
andsio.enter_room
. Instead, we fully embrace into the "rugpull" nature by simply letclient = Client.instances.get(data['client_id'])
to find our made-up client and handle the logic there.Enable
ui.navigate.soft_reload
client keep track of the socket id (for ui.navigate.soft_reload())
It is used for shutting down the Socket.IO connection. Without it, I don't see how it can be possible...
browser to aggressively reconnect for ACID
If I am not mistaken, as of writing, NiceGUI never offered any public or private methods to disconnect the Socket.IO connection gracefully (or in a way that the
disconnect
handler will be called), since thesocket_id
(not any UUID generated by NiceGUI, mind you, but ID generated by Python.SocketIO, which looks likexq8S7tkOWr3iIu-KAAAD
) was never kept track of.Therefore, my argument is, I can simply define the behaviour of disconnect to be an immediate reconnect, without fear it break existing code.
NOTE: When the browser reconnects, it starts over by setting
window.nextMessageId = 0
. This is because, though there may be 1000 messages from the old client, we still want to listen to every single message broadcasted by the new client.ui.navigate.soft_reload() code
It is simply the
try_and_close_the_websocket
in the testing code, baked into the NiceGUI library.The procedure for soft_reload:
Remaining concerns