Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

620 - Update util std functions #625

Merged
merged 4 commits into from
Oct 9, 2018
Merged

620 - Update util std functions #625

merged 4 commits into from
Oct 9, 2018

Conversation

mitsuaki-u
Copy link
Contributor

@mitsuaki-u mitsuaki-u commented Sep 27, 2018

What was the problem?

Related to PR: #622
Race condition in getRawStdin causing inconsistent results when piping.

How did I fix it?

Update getStdin func, remove getRawStdin and promise.race.

How to test it?

./bin/run transaction:create -t=0 100 100L --passphrase="pass:123" | ./bin/run transaction:sign --passphrase="pass:123"

Review checklist

if (stdinIsTTY()) {
reject(new Error(`Timed out after ${DEFAULT_TIMEOUT} ms`));
}
resolve(readFromStd);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's strange to resolve a Promise here.

@mitsuaki-u mitsuaki-u force-pushed the 620-timeout_promise branch from dcd04ed to 11ef6c9 Compare October 4, 2018 15:29
shuse2
shuse2 previously requested changes Oct 4, 2018
@@ -60,7 +64,7 @@ export const getRawStdIn = () => {
.on('line', line => lines.push(line))
.on('close', () => resolve(lines));
});
return Promise.race([readFromStd, timeoutPromise(DEFAULT_TIMEOUT)]);
return Promise.race([readFromStd, timeoutPromise(readFromStd)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

if we wrap the promise, I think we can remove Promise.race

const timeoutPromise = async readFromStd => {
try {
await (() =>
new Promise(([reject]) => setTimeout(reject, DEFAULT_TIMEOUT)))();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we clearTimeout in normal scenario?
Also, this doesn't throw error, so what are we doing for try-catch...? do we need Promise here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't thought about how to clean it up, but intended functionality is something like this?
Maybe we can chain this function to handleClose part so it uses getRawStdIn in getStdIn and it just handles the output nicely?

const readFromStd = new Promise((resolve, reject) => {
	const rl = readline.createInterface({ input: process.stdin });
	const lines = [];
	rl
		.on('line', line => lines.push(line))
		.on('close', () => resolve(lines));

	const id = setTimeout(() => {
		clearTimeout(id);
		if (stdinIsTTY()) {
			reject(new Error(`Timed out after ${DEFAULT_TIMEOUT} ms`));
		} else {
			resolve(lines);
		}
	}, DEFAULT_TIMEOUT);
});
return readFromStd;

@mitsuaki-u mitsuaki-u changed the title 620-Timeout promise handles pipe 620 - Update util std functions Oct 5, 2018
@mitsuaki-u mitsuaki-u force-pushed the 620-timeout_promise branch from 11ef6c9 to 34e87d8 Compare October 5, 2018 15:09
shuse2
shuse2 previously requested changes Oct 5, 2018
@@ -109,7 +98,7 @@ export const getStdIn = ({

return rl.on('line', line => lines.push(line)).on('close', handleClose);
});
return Promise.race([readFromStd, timeoutPromise(DEFAULT_TIMEOUT)]);
return Promise.resolve(readFromStd);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should just return readFromStd here

if (stdinIsTTY()) {
return reject(new Error(`Timed out after ${DEFAULT_TIMEOUT} ms`));
}
return resolve(lines);
Copy link
Contributor

Choose a reason for hiding this comment

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

this content of resolve should be consistent with the other resolve

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's probably easier to change handleClose to function (line: string[]) => { passphrase, second... }, then call from close event and timeout event

Copy link
Contributor

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

LGTM GJ

@mitsuaki-u mitsuaki-u force-pushed the 620-timeout_promise branch from a3f12ed to fcf52ff Compare October 8, 2018 08:59
@shuse2 shuse2 merged commit 6edc7f2 into 2.0.0 Oct 9, 2018
@shuse2 shuse2 deleted the 620-timeout_promise branch October 9, 2018 07:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants