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

Fix hanging "Prebuild in Progress" page #10357

Merged
merged 4 commits into from
Jun 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 16 additions & 28 deletions components/dashboard/src/start/CreateWorkspace.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -461,40 +461,29 @@ interface RunningPrebuildViewProps {
}

function RunningPrebuildView(props: RunningPrebuildViewProps) {
const logsEmitter = new EventEmitter();
let pollTimeout: NodeJS.Timeout | undefined;
let prebuildDoneTriggered: boolean = false;
const [logsEmitter] = useState(new EventEmitter());

useEffect(() => {
const checkIsPrebuildDone = async (): Promise<boolean> => {
if (prebuildDoneTriggered) {
console.debug("prebuild done already triggered, doing nothing");
return true;
}

const done = await getGitpodService().server.isPrebuildDone(props.runningPrebuild.prebuildID);
if (done) {
// note: this treats "done" as "available" which is not equivalent.
// This works because the backend ignores prebuilds which are not "available", and happily starts a workspace as if there was no prebuild at all.
prebuildDoneTriggered = true;
props.onPrebuildSucceeded();
return true;
}
return false;
};
const pollIsPrebuildDone = async () => {
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 was unused at all 🙈

clearTimeout(pollTimeout!);
await checkIsPrebuildDone();
pollTimeout = setTimeout(pollIsPrebuildDone, 10000);
};

const disposables = watchHeadlessLogs(
props.runningPrebuild.instanceID,
(chunk) => logsEmitter.emit("logs", chunk),
checkIsPrebuildDone,
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 was the only option to proceed, i.e. only if headless logs poller checked for isPrebuildDone periodically, we'd a chance to proceed. whenever that poller broke, this was a UX dead-end.

async () => false,
);

disposables.push(
getGitpodService().registerClient({
onInstanceUpdate: (update) => {
if (update.workspaceId !== props.runningPrebuild.workspaceID) {
return;
}
if (update.status.phase === "stopped") {
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 is the old assumption in a new shape 😞
'still don't like it myself, but it kind of works.
what it does: when we enter the RunningPrebuildView scene, we're listening for the stopped event of image-build/prebuild, and when it's happening we proceed.

props.onPrebuildSucceeded();
}
},
}),
);

return function cleanup() {
clearTimeout(pollTimeout!);
disposables.dispose();
};
}, []);
Expand All @@ -507,7 +496,6 @@ function RunningPrebuildView(props: RunningPrebuildViewProps) {
<button
className="mt-6 secondary"
onClick={() => {
clearTimeout(pollTimeout!);
props.onIgnorePrebuild();
}}
>
Expand Down
21 changes: 12 additions & 9 deletions components/dashboard/src/start/StartWorkspace.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { IDEOptions } from "@gitpod/gitpod-protocol/lib/ide-protocol";
import { ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
import EventEmitter from "events";
import * as queryString from "query-string";
import React, { Suspense, useEffect } from "react";
import React, { Suspense, useEffect, useState } from "react";
import { v4 } from "uuid";
import Arrow from "../components/Arrow";
import ContextMenu from "../components/ContextMenu";
Expand Down Expand Up @@ -131,7 +131,13 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
}

try {
this.toDispose.push(getGitpodService().registerClient(this));
this.toDispose.push(
getGitpodService().registerClient({
notifyDidOpenConnection: () => this.fetchWorkspaceInfo(undefined),
onInstanceUpdate: (workspaceInstance: WorkspaceInstance) =>
this.onInstanceUpdate(workspaceInstance),
}),
);
} catch (error) {
console.error(error);
this.setState({ error });
Expand Down Expand Up @@ -297,10 +303,6 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
this.setState({ ideOptions });
}

notifyDidOpenConnection() {
this.fetchWorkspaceInfo(undefined);
}

async onInstanceUpdate(workspaceInstance: WorkspaceInstance) {
if (workspaceInstance.workspaceId !== this.props.workspaceId) {
return;
Expand Down Expand Up @@ -661,19 +663,20 @@ interface ImageBuildViewProps {
}

function ImageBuildView(props: ImageBuildViewProps) {
const logsEmitter = new EventEmitter();
Copy link
Member Author

Choose a reason for hiding this comment

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

If the parent comp of ImageBuildView decided to re-render this will be reinitialized unintentionally.

const [logsEmitter] = useState(new EventEmitter());

useEffect(() => {
let registered = false;
const watchBuild = () => {
if (registered) {
return;
}
registered = true;

getGitpodService()
.server.watchWorkspaceImageBuildLogs(props.workspaceId)
.then(() => (registered = true))
.catch((err) => {
registered = false;
if (err?.code === ErrorCodes.HEADLESS_LOG_NOT_YET_AVAILABLE) {
// wait, and then retry
setTimeout(watchBuild, 5000);
Expand Down Expand Up @@ -718,7 +721,7 @@ function ImageBuildView(props: ImageBuildViewProps) {
}

function HeadlessWorkspaceView(props: { instanceId: string }) {
const logsEmitter = new EventEmitter();
const [logsEmitter] = useState(new EventEmitter());

useEffect(() => {
const disposables = watchHeadlessLogs(
Expand Down
4 changes: 3 additions & 1 deletion components/gitpod-protocol/src/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,9 @@ export interface PrebuiltWorkspace {

export namespace PrebuiltWorkspace {
export function isDone(pws: PrebuiltWorkspace) {
return pws.state === "available" || pws.state === "timeout" || pws.state === "aborted";
return (
Copy link
Member Author

Choose a reason for hiding this comment

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

though isDone seems to be very unused after this PR, it seems to be wrong to have the failed state missing in here.

Copy link
Member

Choose a reason for hiding this comment

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

If it's unused, let's remove it. It's always available in git history.

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 API though.

Copy link
Member

Choose a reason for hiding this comment

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

It's API though.

What do you mean?

pws.state === "available" || pws.state === "timeout" || pws.state === "aborted" || pws.state === "failed"
);
}

export function isAvailable(pws: PrebuiltWorkspace) {
Expand Down