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.

Sign up for GitHub

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

skip Positron Proxy for fastapi and streamlit apps on PWB #5138

Merged
merged 21 commits into from
Oct 29, 2024
Merged

skip Positron Proxy for fastapi and streamlit apps on PWB #5138

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f260290
add log output channel for positron proxy
sharon-wang Oct 23, 2024
7cc22a9
fix handling of custom urlPath in positron-run-app
sharon-wang Oct 23, 2024
aaf999b
remove trailing slashes and add trace logging
sharon-wang Oct 23, 2024
a5d3c2f
add workbench-specific handling to determine the proxy targetOrigin
sharon-wang Oct 23, 2024
c5aca3c
remove removal of trailing slash from target origin
sharon-wang Oct 23, 2024
e817274
remove fastapi workbench-specific workaround
sharon-wang Oct 28, 2024
ec92a71
skip positron proxy for fastapi apps on PWB
sharon-wang Oct 28, 2024
8cf5740
remove fastapi workbench-specific workaround
sharon-wang Oct 28, 2024
9cea3be
update function doc and match style with other functions
sharon-wang Oct 28, 2024
88696b2
add typescript dep to positron-run-app
sharon-wang Oct 28, 2024
1528565
add websocket proxy req logging and explicit upgrade handling
sharon-wang Oct 24, 2024
a2b51ae
override websocket proxy request origin header
sharon-wang Oct 24, 2024
b91c7f5
WIP: extra handling in positron proxy for websockets
sharon-wang Oct 25, 2024
b67a1b5
add debug logging for websocket upgrades
sharon-wang Oct 25, 2024
c8ce6a1
try to rewrite the proxy with http-proxy
sharon-wang Oct 25, 2024
d142b12
Revert "try to rewrite the proxy with http-proxy"
sharon-wang Oct 29, 2024
df57747
Revert "add debug logging for websocket upgrades"
sharon-wang Oct 29, 2024
48d38f8
Revert "WIP: extra handling in positron proxy for websockets"
sharon-wang Oct 29, 2024
8f4713d
Revert "override websocket proxy request origin header"
sharon-wang Oct 29, 2024
465e2f0
Revert "add websocket proxy req logging and explicit upgrade handling"
sharon-wang Oct 29, 2024
d4fc66d
disable positron proxy for streamlit on PWB
sharon-wang Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions extensions/positron-proxy/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ import { PositronProxy } from './positronProxy';
*/
export type ProxyServerStyles = { readonly [key: string]: string | number };

/**
* Positron Proxy log output channel.
*/
export const log = vscode.window.createOutputChannel('Positron Proxy', { log: true });

/**
* Activates the extension.
* @param context An ExtensionContext that contains the extention context.
Expand All @@ -20,6 +25,9 @@ export function activate(context: vscode.ExtensionContext) {
// Create the PositronProxy object.
const positronProxy = new PositronProxy(context);

// Create the log output channel.
context.subscriptions.push(log);

// Register the positronProxy.startHelpProxyServer command and add its disposable.
context.subscriptions.push(
vscode.commands.registerCommand(
Expand Down
55 changes: 44 additions & 11 deletions extensions/positron-proxy/src/positronProxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import fs = require('fs');
import path = require('path');
import express from 'express';
import { AddressInfo, Server } from 'net';
import { ProxyServerStyles } from './extension';
import { log, ProxyServerStyles } from './extension';
// eslint-disable-next-line no-duplicate-imports
import { Disposable, ExtensionContext } from 'vscode';
import { createProxyMiddleware, responseInterceptor } from 'http-proxy-middleware';
import { HtmlProxyServer } from './htmlProxy';
Expand Down Expand Up @@ -176,7 +177,7 @@ export class PositronProxy implements Disposable {
this._helpStyleOverrides !== undefined &&
this._helpScript !== undefined;
} catch (error) {
console.log(`Failed to load the resources/scripts_help.html file.`);
log.error(`Failed to load the resources/scripts_help.html file: ${JSON.stringify(error)}`);
}
}

Expand All @@ -202,6 +203,8 @@ export class PositronProxy implements Disposable {
* @returns The server origin.
*/
startHelpProxyServer(targetOrigin: string): Promise<string> {
log.debug(`Starting a help proxy server for target: ${targetOrigin}...`);

// Start the proxy server.
return this.startProxyServer(
targetOrigin,
Expand Down Expand Up @@ -257,6 +260,8 @@ export class PositronProxy implements Disposable {
* stopped.
*/
stopProxyServer(targetOrigin: string): boolean {
log.debug(`Stopping proxy server for target: ${targetOrigin}...`);

// See if we have a proxy server for the target origin. If we do, stop it.
const proxyServer = this._proxyServers.get(targetOrigin);
if (proxyServer) {
Expand All @@ -278,6 +283,8 @@ export class PositronProxy implements Disposable {
* @returns The server URL.
*/
async startHtmlProxyServer(targetPath: string) {
log.debug(`Starting an HTML proxy server for target: ${targetPath}...`);

if (!this._htmlProxyServer) {
this._htmlProxyServer = new HtmlProxyServer();
}
Expand All @@ -299,6 +306,7 @@ export class PositronProxy implements Disposable {
* @returns The server origin.
*/
startHttpProxyServer(targetOrigin: string): Promise<string> {
log.debug(`Starting an HTTP proxy server for target: ${targetOrigin}...`);
// Start the proxy server.
return this.startProxyServer(targetOrigin, htmlContentRewriter);
}
Expand All @@ -312,6 +320,7 @@ export class PositronProxy implements Disposable {
* @returns The pending proxy server info.
*/
startPendingHttpProxyServer(): Promise<PendingProxyServer> {
log.debug('Starting a pending HTTP proxy server...');
// Start the proxy server and return the pending proxy server info. The caller will need to
// call finishProxySetup to complete the proxy setup.
return this.startNewProxyServer(htmlContentRewriter);
Expand All @@ -332,7 +341,7 @@ export class PositronProxy implements Disposable {
// server origin.
const proxyServer = this._proxyServers.get(targetOrigin);
if (proxyServer) {
console.debug(`Existing proxy server ${proxyServer.serverOrigin} found for target: ${targetOrigin}.`);
log.debug(`Existing proxy server ${proxyServer.serverOrigin} found for target: ${targetOrigin}.`);
return proxyServer.serverOrigin;
}

Expand All @@ -341,20 +350,21 @@ export class PositronProxy implements Disposable {
// We don't have an existing proxy server for the target origin, so start a new one.
pendingProxy = await this.startNewProxyServer(contentRewriter);
} catch (error) {
console.error(`Failed to start a proxy server for ${targetOrigin}.`);
log.error(`Failed to start a proxy server for ${targetOrigin}: ${JSON.stringify(error)}`);
throw error;
}

const externalUri = pendingProxy.externalUri.toString(true);
try {
// Finish setting up the proxy server.
await pendingProxy.finishProxySetup(targetOrigin);
} catch (error) {
console.error(`Failed to finish setting up the proxy server at ${pendingProxy.externalUri} for target: ${targetOrigin}.`);
log.error(`Failed to finish setting up the proxy server at ${externalUri} for target ${targetOrigin}: ${JSON.stringify(error)}`);
throw error;
}

// Return the external URI.
return pendingProxy.externalUri.toString();
return externalUri;
}

/**
Expand All @@ -377,8 +387,10 @@ export class PositronProxy implements Disposable {

// Ensure the address is an AddressInfo.
if (!isAddressInfo(address)) {
const error = `Failed to get the address info ${JSON.stringify(address)} for the server.`;
log.error(error);
server.close();
throw new Error(`Failed to get the address info ${JSON.stringify(address)} for the server.`);
throw new Error(error);
}

// Create the server origin.
Expand All @@ -388,6 +400,8 @@ export class PositronProxy implements Disposable {
const originUri = vscode.Uri.parse(serverOrigin);
const externalUri = await vscode.env.asExternalUri(originUri);

log.debug(`Started proxy server at ${serverOrigin} for external URI ${externalUri.toString(true)}.`);

// Return the pending proxy info.
return {
externalUri: externalUri,
Expand Down Expand Up @@ -423,6 +437,11 @@ export class PositronProxy implements Disposable {
app: express.Express,
contentRewriter: ContentRewriter
) {
log.debug(`Finishing proxy server setup for target ${targetOrigin}\n` +
`\tserverOrigin: ${serverOrigin}\n` +
`\texternalUri: ${externalUri.toString(true)}`
);

// Add the proxy server.
this._proxyServers.set(targetOrigin, new ProxyServer(
serverOrigin,
Expand All @@ -436,16 +455,30 @@ export class PositronProxy implements Disposable {
changeOrigin: true,
selfHandleResponse: true,
ws: true,
// Logging for development work.
// onProxyReq: (proxyReq, req, res, options) => {
// console.log(`Proxy request ${serverOrigin}${req.url} -> ${targetOrigin}${req.url}`);
// },
onProxyReq: (proxyReq, req, _res, _options) => {
log.trace(`onProxyReq - proxy request ${serverOrigin}${req.url} -> ${targetOrigin}${req.url}` +
`\n\tmethod: ${proxyReq.method}` +
`\n\tprotocol: ${proxyReq.protocol}` +
`\n\thost: ${proxyReq.host}` +
`\n\turl: ${proxyReq.path}` +
`\n\theaders: ${JSON.stringify(proxyReq.getHeaders())}` +
`\n\texternal uri: ${externalUri.toString(true)}`
);
},
onProxyRes: responseInterceptor(async (responseBuffer, proxyRes, req, _res) => {
log.trace(`onProxyRes - proxy response ${targetOrigin}${req.url} -> ${serverOrigin}${req.url}` +
`\n\tstatus: ${proxyRes.statusCode}` +
`\n\tstatusMessage: ${proxyRes.statusMessage}` +
`\n\theaders: ${JSON.stringify(proxyRes.headers)}` +
`\n\texternal uri: ${externalUri.toString(true)}`
);

// Get the URL and the content type. These must be present to call the
// content rewriter. Also, the scripts must be loaded.
const url = req.url;
const contentType = proxyRes.headers['content-type'];
if (!url || !contentType || !this._scriptsFileLoaded) {
log.trace(`onProxyRes - skipping response processing for ${serverOrigin}${url}`);
// Don't process the response.
return responseBuffer;
}
Expand Down
19 changes: 14 additions & 5 deletions extensions/positron-run-app/src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,18 +353,27 @@ async function previewUrlInExecutionOutput(execution: vscode.TerminalShellExecut
return false;
}

// Convert the url to an external URI.
// Example: http://localhost:8500
const localBaseUri = vscode.Uri.parse(url.toString());

// Example: http://localhost:8500/url/path or http://localhost:8500
const localUri = urlPath ?
vscode.Uri.joinPath(localBaseUri, urlPath) : localBaseUri;

log.debug(`Viewing app at local uri: ${localUri} with external uri ${proxyInfo.externalUri.toString()}`);
// Determine the target origin based on whether the server is running in PWB.
const runningInPWB = process.env.RS_SERVER_URL ? true : false;
sharon-wang marked this conversation as resolved.
Show resolved Hide resolved
const targetOrigin = runningInPWB ? localBaseUri : localUri;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I'm curious why the target origin differs here for workbench

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of a hack as I couldn't get UVICORN_ROOT_PATH or --root-path to work for fastapi in Positron Server Web or Desktop. I haven't gotten to the bottom of why it's working on Workbench.

The result on Positron Server Web without this handling is that the openapi.json file is getting loaded from the root /openapi.json instead of from e.g. proxy/1234/openapi.json, so we get:

positron_web_fastapi_openapi_notfound

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna move this PR to Draft while I try a few more things to see if we can avoid this custom handling.

Copy link
Member Author

@sharon-wang sharon-wang Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Positron Server Web still needs this workaround. I still don't understand why the root path setting doesn't work.

I've decided to not pass fastapi through the Positron Proxy for now. I'll probably do the same for Streamlit as well.

await proxyInfo.finishProxySetup(targetOrigin.toString(true));

// Example: http://localhost:8080/proxy/5678/url/path or http://localhost:8080/proxy/5678
const previewUri = runningInPWB && urlPath
? vscode.Uri.joinPath(proxyInfo.externalUri, urlPath)
: proxyInfo.externalUri;

// Finish the Positron proxy setup so that proxy middleware is hooked up.
await proxyInfo.finishProxySetup(localUri.toString());
log.debug(`Viewing app at local uri ${localUri} with external uri ${previewUri.toString(true)}`);

// Preview the external URI.
positron.window.previewUrl(proxyInfo.externalUri);
positron.window.previewUrl(previewUri);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seeM When running Fastapi in Positron on Workbench, sometimes the app url is previewed in the Viewer slightly too early, before the app server is ready. To get around this, a user can refresh the Viewer, but that is not ideal if it happens every time.

Do you have thoughts on how to get around this issue? Maybe we can ping the uri to check if it's responsive before displaying it in the Viewer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe #4276 is describing what I'm seeing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that uvicorn would only print the URL to stdout once the server is ready to receive connections (I haven't confirmed that). Is the delay happening at the Positron Proxy layer? If so, maybe there's something we could expose from the Positron Proxy extension i.e. an awaitable or an event that we could listen to here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this a bit more and it does not seem to be in the Positron Proxy layer, as the issue still occurs even if we don't use the Positron Proxy.

For fastapi with uvicorn, we may want to wait for Application startup complete in the terminal output before we open the app at the url. We happen to be just a little quick to open the url/slow to startup the app on Workbench, such that the fastapi app is not quite ready yet.

Perhaps this would be a separate property in the terminal/debug config for some terminal text that indicates the app is ready, before we extract the app url and view it. I'll open a separate issue for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #5187


return true;
}
Expand Down