Allow Livebook to proxy requests to the runtime #2608
Conversation
aleDsz
commented
May 16, 2024
aleDsz
commented
- Closes Allow Livebook to proxy requests to the runtime #2090
|
Uffizzi Preview |
lib/livebook_web/plugs/proxy_plug.ex
Outdated
|
|
||
| def call(%{path_info: ["apps", slug, "proxy" | path_info]} = conn, _opts) do | ||
| app = fetch_app!(slug) | ||
| id = App.get_session_id(app.pid) |
There was a problem hiding this comment.
@josevalim should we validate that the app is single-session? Also, auto_shutdown_ms would be an issue for the first request.
There was a problem hiding this comment.
Good call. We should validate single session app here but also add a route to support multi-session apps. What would be the URL in this case? This?
def call(%{path_info: ["apps", slug, session_id, "proxy" | path_info]} = conn, _opts) do
There was a problem hiding this comment.
Technically each app session has that URL, for single-session we redirect the user to that specific URL right away.
The main concern is hitting the app at start, while the proxy is not started yet. So your idea is that the user would start the multi-session app session, perhaps enter some data, and then they could query that session's proxy?
There was a problem hiding this comment.
Yes, exactly. We need to document on Kino.Proxy.listen that:
- For single session apps, you can access it at
/apps/slug/proxyfor the other apps and session, you need the session URL - You must not enable automatic shutdowns, we don't automatically start apps on demand at the moment
There was a problem hiding this comment.
we don't automatically start apps on demand at the moment
With the current code we do, but they need time to setup :D So should we change it to only use existing single-session and error otherwise?
There was a problem hiding this comment.
Hrm... I am worried someone may have a never completing app and then those would never work under any URL and having a distinction between them could be helpful? I don't know.
There was a problem hiding this comment.
Like, given we need to have a session_id, it means you need to manually go and boot it up, I think it is fine to assume it is running.
There was a problem hiding this comment.
Anyway, we can decide this later. Let's keep this on the path to merging now. I will resolve the conversation and drop a more precise feedback for Ale.
There was a problem hiding this comment.
it means you need to manually go and boot it up
For single session it's just started on deployment, so there's a race condition when someone hits the app too soon.
I am worried someone may have a never completing app
I would error on errors and interrupts, if it takes forever to execute, then it's actually something the user should know and fix, no?
There was a problem hiding this comment.
I guess it is fair and the request will eventually timeout anyway. But yeah, let's implement this later. :)
|
LGTM :) |
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
