Resolve copy promises

[sjawhar]

The promise returned by cpToPod never resolves. Followed the example from here to fix: #982 (comment)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sjawhar
Once this PR has been reviewed and has the lgtm label, please assign brendandburns for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: sjawhar / name: Sami Jawhar (10c7f1c, 5340328, 913a021, eef93d6, 5fc727f)

[k8s-ci-robot]

Welcome @sjawhar!

It looks like this is your first PR to kubernetes-client/javascript 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/javascript has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[mstruebing]

Hey @sjawhar thanks for your pr but I think the tests are broken, could you have a look?
You can run them locally via npm test

[brendandburns]

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.

[sjawhar]

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.

[sjawhar]

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

[brendandburns]

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.

[sjawhar]

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

[sjawhar]

Hey @sjawhar thanks for your pr but I think the tests are broken, could you have a look? You can run them locally via npm test

Yeah, sorry about that, I had quickly made the edits in the browser to make sure I didn't forget. Now I've actually, like, tested it and stuff 😅

[cjihrig]

Is there a test that can be added for this?

[sjawhar]

Is there a test that can be added for this?

I've spend some time on it but didn't figure out the different mocks well enough to understand why the tests were passing before instead of timing out as expected. The lines in the test that send a V1Status seem not to match actual behavior. Removing them causes the cp promise to never resolve (even after the fix), so some other change is also needed.

If it helps at all, I've read through the code and set breakpoints to interactively debug and this the issue largely comes down to this:

javascript/src/web-socket-handler.ts

Lines 55 to 73 in 88da3bd

	public static handleStandardInput(
	ws: WebSocket.WebSocket,
	stdin: stream.Readable | any,
	streamNum: number = 0,
	): boolean {
	stdin.on('data', (data) => {
	const buff = Buffer.alloc(data.length + 1);
	buff.writeInt8(streamNum, 0);
	if (data instanceof Buffer) {
	data.copy(buff, 1);
	} else {
	buff.write(data, 1);
	}
	ws.send(buff);
	});
	stdin.on('end', () => {
	ws.close();
	});

It looks to me like when stdin is provided, the websocket is closed without waiting for a StatusStream response, so the status callback never gets called. Maybe that'll help you fix the tests, or maybe the right fix is actually upstream of this logic (maybe #2037 ?)