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

Resolve copy promises #2038

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Resolve copy promises #2038

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5340328
Resolve copy promises
sjawhar Nov 20, 2024
913a021
Update cp.ts
sjawhar Nov 20, 2024
10c7f1c
Update cp.ts
sjawhar Nov 20, 2024
eef93d6
.on('close') instead of .onclose =
sjawhar Nov 23, 2024
5fc727f
Don't resolve twice
sjawhar Nov 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
10 changes: 10 additions & 0 deletions src/cp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ export class Cp {
}
},
)
.then((conn) => {
conn.on('close', () => {
resolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a resolve above at line 63. I don't think that this resolve is required. In fact I'm not sure it's what we want, because this will resolve when the websocket is returned. We want to resolve when the copy completes.

Copy link
Author

@sjawhar sjawhar Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That callback only gets called if status != null:

javascript/src/exec.ts

Lines 53 to 62 in 8db906a

const conn = await this.handler.connect(path, null, (streamNum: number, buff: Buffer): boolean => {
const status = WebSocketHandler.handleStandardStreams(streamNum, buff, stdout, stderr);
if (status != null) {
if (statusCallback) {
statusCallback(status);
}
return false;
}
return true;
});

For copies (or maybe for all exec calls that use stdin), status == null, so the callback never gets called, so the promise never resolves. awaiting calls to cpToPod hang forever.

Copy link
Author

@sjawhar sjawhar Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If the copy isn't complete when the websocket connection is closed, how can the caller ever know that it has completed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed that you are doing this on the 'close' event. My mistake. But we should probably make sure that we don't resolve the promise twice.

Copy link
Author

Choose a reason for hiding this comment

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

OK, added a check for that in a way that shouldn't cause things to break if changes elsewhere cause this logic to change.

});
})
.catch(reject);
});
}
Expand Down Expand Up @@ -115,6 +120,11 @@ export class Cp {
}
},
)
.then((conn) => {
conn.on('close', () => {
resolve();
});
})
.catch(reject);
});
}
Expand Down