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

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

Merged
merged 21 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from 10 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
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
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
3 changes: 2 additions & 1 deletion extensions/positron-run-app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
"@types/mocha": "^10.0.8",
"@types/node": "^22.5.4",
"@types/sinon": "^17.0.3",
"@types/sinon-test": "^2.4.6"
"@types/sinon-test": "^2.4.6",
"typescript": "^4.5.5"
Comment on lines +42 to +43
Copy link
Member Author

Choose a reason for hiding this comment

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

this allows us to run vsce package in the extensions/positron-run-app directory to generate a VSIX, which can be loaded into Positron. This is helpful for faster extension dev iteration when running a pre-built reh-web version of Positron.

},
"repository": {
"type": "git",
Expand Down
81 changes: 67 additions & 14 deletions extensions/positron-run-app/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import { raceTimeout, removeAnsiEscapeCodes, SequencerByKey } from './utils';
// Regex to match a URL with the format http://localhost:1234/path
const localUrlRegex = /http:\/\/(localhost|127\.0\.0\.1):(\d{1,5})(\/[^\s]*)?/;

const isPositronWeb = vscode.env.uiKind === vscode.UIKind.Web;
const isRunningOnPwb = !!process.env.RS_SERVER_URL && isPositronWeb;

type PositronProxyInfo = {
proxyPath: string;
externalUri: vscode.Uri;
Expand Down Expand Up @@ -88,12 +91,18 @@ export class PositronRunAppApiImpl implements PositronRunApp, vscode.Disposable
return;
}

// Set up the proxy server for the application if applicable.
let urlPrefix = undefined;
let proxyInfo: PositronProxyInfo | undefined;
if (shouldUsePositronProxy(options.name)) {
// Start the proxy server
proxyInfo = await vscode.commands.executeCommand<PositronProxyInfo>('positronProxy.startPendingProxyServer');
log.debug(`Proxy started for app at proxy path ${proxyInfo.proxyPath} with uri ${proxyInfo.externalUri.toString()}`);
urlPrefix = proxyInfo.proxyPath;
}

// Get the terminal options for the application.
progress.report({ message: vscode.l10n.t('Getting terminal options...') });
// Start the proxy server
const proxyInfo = await vscode.commands.executeCommand<PositronProxyInfo>('positronProxy.startPendingProxyServer');
log.debug(`Proxy started for app at proxy path ${proxyInfo.proxyPath} with uri ${proxyInfo.externalUri.toString()}`);
const urlPrefix = proxyInfo.proxyPath;
const terminalOptions = await options.getTerminalOptions(runtime, document, urlPrefix);
if (!terminalOptions) {
return;
Expand Down Expand Up @@ -221,11 +230,18 @@ export class PositronRunAppApiImpl implements PositronRunApp, vscode.Disposable
return;
}

// Set up the proxy server for the application if applicable.
let urlPrefix = undefined;
let proxyInfo: PositronProxyInfo | undefined;
if (shouldUsePositronProxy(options.name)) {
// Start the proxy server
proxyInfo = await vscode.commands.executeCommand<PositronProxyInfo>('positronProxy.startPendingProxyServer');
log.debug(`Proxy started for app at proxy path ${proxyInfo.proxyPath} with uri ${proxyInfo.externalUri.toString()}`);
urlPrefix = proxyInfo.proxyPath;
}

// Get the debug config for the application.
progress.report({ message: vscode.l10n.t('Getting debug configuration...') });
// Start the proxy server
const proxyInfo = await vscode.commands.executeCommand<PositronProxyInfo>('positronProxy.startPendingProxyServer');
const urlPrefix = proxyInfo.proxyPath;
const debugConfig = await options.getDebugConfiguration(runtime, document, urlPrefix);
if (!debugConfig) {
return;
Expand Down Expand Up @@ -328,7 +344,7 @@ export class PositronRunAppApiImpl implements PositronRunApp, vscode.Disposable
}
}

async function previewUrlInExecutionOutput(execution: vscode.TerminalShellExecution, proxyInfo: PositronProxyInfo, urlPath?: string) {
async function previewUrlInExecutionOutput(execution: vscode.TerminalShellExecution, proxyInfo?: PositronProxyInfo, urlPath?: string) {
// Wait for the server URL to appear in the terminal output, or a timeout.
const stream = execution.read();
const url = await raceTimeout(
Expand All @@ -353,18 +369,33 @@ 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()}`);
// Example: http://localhost:8080/proxy/5678/url/path or http://localhost:8080/proxy/5678
let previewUri = undefined;
if (proxyInfo) {
// On Web (specifically Positron Server Web and not PWB), we need to set up the proxy with
// the urlPath appended to avoid issues where the app does not set the base url of the app
// or the base url of referenced assets correctly.
const applyWebPatch = isPositronWeb && !isRunningOnPwb;
const targetOrigin = applyWebPatch ? localUri.toString(true) : localBaseUri.toString();

// Finish the Positron proxy setup so that proxy middleware is hooked up.
await proxyInfo.finishProxySetup(targetOrigin);
previewUri = !applyWebPatch && urlPath ? vscode.Uri.joinPath(proxyInfo.externalUri, urlPath) : proxyInfo.externalUri;
} else {
previewUri = await vscode.env.asExternalUri(localUri);
}

// 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.toString(true)} with external uri ${previewUri.toString(true)}`);

// Preview the external URI.
positron.window.previewUrl(proxyInfo.externalUri);
// Preview the app in the Viewer.
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 Expand Up @@ -442,3 +473,25 @@ async function showShellIntegrationNotSupportedMessage(): Promise<void> {
await runAppConfig.update('showShellIntegrationNotSupportedMessage', false, vscode.ConfigurationTarget.Global);
}
}

/**
* Check if the Positron proxy should be used for the given app.
* Generally, we should avoid skipping the proxy unless there is a good reason to do so, as the
* proxy gives us the ability to intercept requests and responses to the app, which is useful for
* things like debugging, applying styling or fixing up urls.
* @param appName The name of the app; indicated in extensions/positron-python/src/client/positron/webAppCommands.ts
* @returns Whether to use the Positron proxy for the app.
*/
function shouldUsePositronProxy(appName: string) {
switch (appName.trim().toLowerCase()) {
case 'fastapi':
// FastAPI apps don't work in Positron on Workbench when run through the proxy.
if (isRunningOnPwb) {
return false;
}
return true;
default:
// By default, proxy the app.
return true;
}
}
5 changes: 5 additions & 0 deletions extensions/positron-run-app/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@
resolved "https://registry.yarnpkg.com/@types/sinonjs__fake-timers/-/sinonjs__fake-timers-8.1.5.tgz#5fd3592ff10c1e9695d377020c033116cc2889f2"
integrity sha512-mQkU2jY8jJEF7YHjHvsQO8+3ughTL1mcnn96igfhONmR+fUPSKIkefQYpSe8bsly2Ep7oQbn/6VG5/9/0qcArQ==

typescript@^4.5.5:
version "4.9.5"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-4.9.5.tgz#095979f9bcc0d09da324d58d03ce8f8374cbe65a"
integrity sha512-1FXk9E2Hm+QzZQ7z+McJiHL4NW1F2EzMu9Nq9i3zAaGqibafqYwCVU6WyWAuyQRRzOlxou8xZSyXLEN8oKj24g==

undici-types@~6.19.2:
version "6.19.8"
resolved "https://registry.yarnpkg.com/undici-types/-/undici-types-6.19.8.tgz#35111c9d1437ab83a7cdc0abae2f26d88eda0a02"
Expand Down