Skip to content

Commit

Permalink
Improve how we retry the websocket connection (#1118)
Browse files Browse the repository at this point in the history
* Make websocket connection retry forever for remote as well. And show an error
message when there have been too many retries.

* Allow CORS for localhost when using node (required for things like /healthz checks) and remove unused check_origin method from request handlers (it only exists for WebSocketHandlers).

* Convert Resolver to TS

* Make WebsocketConnection ping via HTTP, and simplify FSM.

* Add docs/examples on how to run streamlit reports on AWS nvidia-docker (#1065)

* Add benchmark.py from https://databricks.com/tensorflow/using-a-gpu

* Make tensorflow not gobble up all the GPU memory.

* Add docker gpu example code.

* Add more Dockerfile comments.

* Turn into streamlit report.

* Add docs

* Make network errors not clear out the whole report.

* Comment

* Remove unused number arg from onConnectionStateChange.

* Rename

* Return successful uri index after ping.

* Comment.

* Comment.

* Rename

* Add ping to Websocket. Also other Tornado settings, like better size limit.

* Show useful error message when connection fails.

* Change a single word in an error message

* [Fix #1097] Use secure websockets when https

* Allow WS CORS from non-localhost when using Node.

* Improve error handling / timeout handling.

* Renames and comments.

* Fix comments from kantuni
  • Loading branch information
tvst committed Jul 22, 2019
1 parent 2f25b5b commit 31406b2
Show file tree
Hide file tree
Showing 11 changed files with 617 additions and 597 deletions.
53 changes: 21 additions & 32 deletions frontend/src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class App extends PureComponent {
this.state = {
reportId: '<null>',
reportName: null,
elements: fromJS([makeElementWithInfoText('Connecting...')]),
elements: fromJS([makeElementWithInfoText('Please wait...')]),
userSettings: {
wideMode: false,
runOnSave: false,
Expand Down Expand Up @@ -136,33 +136,17 @@ class App extends PureComponent {
if (newState === ConnectionState.CONNECTED) {
logMessage('Reconnected to server; Requesting a run (which may be preheated)')
this.widgetMgr.sendUpdateWidgetsMessage()
this.setState({ dialog: null })
}
}

showError(error, info) {
logError(error, info)
showError(errorNode) {
logError(errorNode)

const errorStr = info == null ?
`${error}` :
`${error}.\n${info}`

this.showSingleTextElement(errorStr, TextProto.Format.ERROR)
}

/**
* Resets the state of client to an empty report containing a single
* element which is an alert of the given type.
*
* body - The message to display
* format - One of the accepted formats from Text.proto.
*/
showSingleTextElement(body, format) {
this.setState({
reportId: '<null>',
elements: fromJS([{
type: 'text',
text: { format, body },
}]),
this.openDialog({
type: 'warning',
title: 'Connection error',
msg: errorNode,
})
}

Expand Down Expand Up @@ -404,13 +388,18 @@ class App extends PureComponent {
} else {
this.openDialog({
type: 'warning',
title: 'Error sharing report',
msg: (
<div>
You do not have sharing configured.
Please contact&nbsp;
<a href="mailto:hello@streamlit.io">Streamlit Support</a>
&nbsp;to setup sharing.
</div>
<React.Fragment>
<div>
You do not have sharing configured.
</div>
<div>
Please contact{' '}
<a href="mailto:hello@streamlit.io">Streamlit Support</a>
{' '}to setup sharing.
</div>
</React.Fragment>
),
})
}
Expand Down Expand Up @@ -540,8 +529,8 @@ class App extends PureComponent {
/**
* Updates the report body when there's a connection error.
*/
handleConnectionError(errMsg) {
this.showError(`Connection error: ${errMsg}`)
handleConnectionError(errNode) {
this.showError(errNode)
}

/**
Expand Down
18 changes: 5 additions & 13 deletions frontend/src/components/core/StatusWidget/StatusWidget.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -357,20 +357,12 @@ export class StatusWidget extends PureComponent<Props, State> {
private static getConnectionStateUI(state: ConnectionState): ConnectionStateUI | undefined {
switch (state) {
case ConnectionState.INITIAL:
case ConnectionState.INITIAL_CONNECTING:
case ConnectionState.PINGING_SERVER:
case ConnectionState.CONNECTING:
return {
icon: <use href={openIconic + '#ellipses'}/>,
label: 'Waiting',
tooltip: 'Waiting for connection',
}

case ConnectionState.DISCONNECTED:
case ConnectionState.RECONNECTING:
case ConnectionState.WAITING:
return {
icon: <use href={openIconic + '#circle-x'}/>,
label: 'Disconnected',
tooltip: 'Disconnected from live data feed',
label: 'Connecting',
tooltip: 'Connecting to Streamlit server',
}

case ConnectionState.CONNECTED:
Expand All @@ -382,7 +374,7 @@ export class StatusWidget extends PureComponent<Props, State> {
return {
icon: <use href={openIconic + '#warning'}/>,
label: 'Error',
tooltip: 'Unable to connect',
tooltip: 'Unable to connect to Streamlit server',
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ function uploadedDialog(props: UploadedProps): ReactElement {

interface WarningProps {
type: 'warning';
title: string;
msg: ReactNode;
onClose: PlainEventHandler;
}
Expand All @@ -280,6 +281,7 @@ interface WarningProps {
function warningDialog(props: WarningProps): ReactElement {
return (
<BasicDialog onClose={props.onClose}>
<ModalHeader>{props.title}</ModalHeader>
<ModalBody>{props.msg}</ModalBody>
<ModalFooter>
<Button outline onClick={props.onClose}>Done</Button>
Expand Down
56 changes: 32 additions & 24 deletions frontend/src/lib/ConnectionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,20 @@ import url from 'url'
import {ConnectionState} from './ConnectionState'
import {ForwardMsg} from 'autogen/protobuf'
import {IS_DEV_ENV, WEBSOCKET_PORT_DEV} from './baseconsts'
import {ReactNode} from 'react'
import {StaticConnection} from './StaticConnection'
import {WebsocketConnection} from './WebsocketConnection'
import {configureCredentials, getObject} from './s3helper'
import {logError} from './log'


/**
* When the websocket connection retries this many times, we show a dialog
* letting the user know we're having problems connecting.
*/
const RETRY_COUNT_FOR_WARNING = 30 // around 15s


interface Props {
/**
* Function that shows the user a login box and returns a promise which
Expand All @@ -30,7 +39,7 @@ interface Props {
/**
* Function to be called when the connection errors out.
*/
onConnectionError: (errorMessage: string) => void;
onConnectionError: (errNode: ReactNode) => void;

/**
* Called when our ConnectionState is changed.
Expand Down Expand Up @@ -83,6 +92,7 @@ export class ConnectionManager {
this.connection = await this.connectToRunningServer()
}
} catch (err) {
logError(err.message)
this.setConnectionState(
ConnectionState.DISCONNECTED_FOREVER, err.message)
}
Expand All @@ -94,28 +104,30 @@ export class ConnectionManager {
this.props.connectionStateChanged(connectionState)
}

if (connectionState === ConnectionState.DISCONNECTED_FOREVER) {
if (errMsg || connectionState === ConnectionState.DISCONNECTED_FOREVER) {
this.props.onConnectionError(errMsg || 'unknown')
}
}

private showRetryError = (totalRetries: number, latestError: ReactNode): void => {
if (totalRetries === RETRY_COUNT_FOR_WARNING) {
this.props.onConnectionError(latestError)
}
}

private connectToRunningServer(): WebsocketConnection {
// If dev, always connect to 8501, since window.location.port is the Node
// server's port 3000.
// If changed, also change config.py
const hostname = window.location.hostname
const port = IS_DEV_ENV ? WEBSOCKET_PORT_DEV : +window.location.port
const uri = getWsUrl(hostname, port)
const host = window.location.hostname
const port = IS_DEV_ENV ? WEBSOCKET_PORT_DEV : Number(window.location.port)
const baseUriParts = { host, port }

return new WebsocketConnection({
uriList: [
//getWsUrl('1.1.1.1', '9999', 'bad'), // Uncomment to test timeout.
//getWsUrl('1.1.1.1', '9999', 'bad2'), // Uncomment to test timeout.
uri,
],
baseUriPartsList: [baseUriParts],
onMessage: this.props.onMessage,
onConnectionStateChange: this.setConnectionState,
isLocal: hostname === 'localhost',
onConnectionStateChange: s => this.setConnectionState(s),
onRetry: this.showRetryError,
})
}

Expand All @@ -136,18 +148,18 @@ export class ConnectionManager {
configuredServerAddress, internalServerIP, externalServerIP, serverPort,
} = manifest

const uriList = configuredServerAddress ?
[getWsUrl(configuredServerAddress, serverPort)] :
const baseUriPartsList = configuredServerAddress ?
[{ host: configuredServerAddress, port: serverPort }] :
[
getWsUrl(externalServerIP, serverPort),
getWsUrl(internalServerIP, serverPort),
{ host: externalServerIP, port: serverPort },
{ host: internalServerIP, port: serverPort },
]

return new WebsocketConnection({
uriList,
baseUriPartsList,
onMessage: this.props.onMessage,
onConnectionStateChange: this.setConnectionState,
isLocal: false,
onConnectionStateChange: s => this.setConnectionState(s),
onRetry: this.showRetryError,
})
}

Expand All @@ -156,7 +168,7 @@ export class ConnectionManager {
manifest,
reportId,
onMessage: this.props.onMessage,
onConnectionStateChange: this.setConnectionState,
onConnectionStateChange: s => this.setConnectionState(s),
})
}

Expand Down Expand Up @@ -207,7 +219,3 @@ async function fetchManifest(reportId: string): Promise<any> {
const data = await getObject({Bucket: bucket, Key: manifestKey})
return data.json()
}

function getWsUrl(host: string, port: number): string {
return `ws://${host}:${port}/stream`
}
6 changes: 2 additions & 4 deletions frontend/src/lib/ConnectionState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@

export enum ConnectionState {
CONNECTED = 'CONNECTED',
DISCONNECTED = 'DISCONNECTED',
DISCONNECTED_FOREVER = 'DISCONNECTED_FOREVER',
INITIAL = 'INITIAL',
INITIAL_CONNECTING = 'INITIAL_CONNECTING', // Treat differently in the UI.
RECONNECTING = 'RECONNECTING',
PINGING_SERVER = 'PINGING_SERVER',
CONNECTING = 'CONNECTING',
STATIC = 'STATIC',
WAITING = 'WAITING',
}
19 changes: 0 additions & 19 deletions frontend/src/lib/Resolver.js

This file was deleted.

25 changes: 25 additions & 0 deletions frontend/src/lib/Resolver.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* @license
* Copyright 2018 Streamlit Inc. All rights reserved.
*/


/**
* A promise wrapper that makes resolve/reject functions public.
*/
export default class Resolver<T> {
public resolve: (arg?: T) => (void|Promise<any>)
public reject: (reason?: any) => (void|Promise<any>)
public promise: Promise<T>

public constructor() {
// Initialize to something so TS is happy.
this.resolve = () => {}
this.reject = () => {}

this.promise = new Promise<T>((resFn, rejFn) => {
this.resolve = resFn
this.reject = rejFn
})
}
}
3 changes: 1 addition & 2 deletions frontend/src/lib/StaticConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,8 @@ interface Props {

/**
* Function called when our ConnectionState changes.
* If the new ConnectionState is ERROR, errMsg will be defined.
*/
onConnectionStateChange: (connectionState: ConnectionState, errMsg?: string) => void;
onConnectionStateChange: (connectionState: ConnectionState) => void;
}

/**
Expand Down

0 comments on commit 31406b2

Please sign in to comment.