Skip to content

Conversation

@webmaster128
Copy link
Contributor

No description provided.

@webmaster128 webmaster128 requested a review from ethanfrey July 20, 2018 13:22
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

This works quite nice, what I was looking for :)

One problem is it seems to die on errors (not just show them, but kill ts-node). This can be fixed in another PR, but I will document in the comments.

throw new Error("Recipient count starts at 1");
}
while (profile.getIdentities(0).length < n + 1) {
// will make identities if needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

nice cleanup

const faucet = faucetId(profile);
const rcpt = await recipient(profile, 4);
const rcptAddr = writer.keyToAddress(chainId, rcpt.pubkey);
const faucet = await getOrCreateIdentity(profile, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is clearer here

# this assumes it was run after bov_init.sh and this exists
DIR="${HOME}/bovtest/${TM_VERSION}"
if [ ! -d "${DIR}" ]; then
if [ ! -d "${BOV_DIR}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

ahh... nice that you pull this into travis.sh, but maybe we can provide a default here for local testing (so I don't have to always type this).

like BOV_DIR=${BOV_DIR:-/tmp/bovtest}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having a static default brings back the problem of re-using the directory that contains files owned by root.

The local way to go is probably to automatically set BOV_DIR and TM_DIR to a new dir when you open a session. Or when booting the machine. I don't know. But I guess it is better to handle this outside of the repo.

@ethanfrey
Copy link
Contributor

It works great until I have a typo, then ts-node dies rather than show the error :(

yarn web4
.load init
> bnsConnector("http://localhost:25555")
Promise {
  <pending>,
  domain:
   Domain {
     domain: null,
     _events:
      { removeListener: [Function: updateExceptionCapture],
        newListener: [Function: updateExceptionCapture],
        error: [Function: debugDomainError] },
     _eventsCount: 3,
     _maxListeners: undefined,
     members: [] } }
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. 

The weird thing is that I tried to reproduce this with a normal error, and it wouldn't die.

yarn web4
.load init
> const die = () => {throw new Error('die');};
undefined
> const adie = async () => {throw new Error('async die');};
undefined
> die()
[eval].ts:1
const die = () => { throw new Error('die'); };
                    ^

Error: die
    at die ([eval].ts:1:27)
    at [eval].ts:1:1
    at Script.runInThisContext (vm.js:91:20)
    at exec (/Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:228:17)
    at /Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:218:27
    at Array.reduce (<anonymous>)
    at _eval (/Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:217:18)
    at REPLServer.replEval (/Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:290:14)
    at bound (domain.js:396:14)
    at REPLServer.runBound [as eval] (domain.js:409:12)
> adie()
Promise {
  <rejected> Error: async die
    at [eval].ts:1:11
    at Generator.next (<anonymous>)
    at [eval].ts:6:71
    at new Promise (<anonymous>)
    at __awaiter ([eval].ts:2:12)
    at adie ([eval].ts:1:20)
    at [eval].ts:1:1
    at Script.runInThisContext (vm.js:91:20)
    at exec (/Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:228:17)
    at /Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:218:27,
  domain:
   Domain {
     domain: null,
     _events:
      { removeListener: [Function: updateExceptionCapture],
        newListener: [Function: updateExceptionCapture],
        error: [Function: debugDomainError] },
     _eventsCount: 3,
     _maxListeners: undefined,
     members: [] } }

Note that wait(adie()) hangs forever... does it don't properly resolve error case?

@ethanfrey
Copy link
Contributor

ethanfrey commented Jul 20, 2018

> const data = async () => 77;
undefined
> const adie = async () => {throw new Error('async die');};
undefined
> wait(data())
77
> wait(adie())
... 60 seconds and counting....
> const adie = async () => {throw new Error('async die');};
undefined
> wait(adie().catch(() => console.log('catch')));
catch
undefined

@webmaster128
Copy link
Contributor Author

webmaster128 commented Jul 20, 2018

  1. unhandled promise rejections are not supposed to be handled. Confirm
$ node
> Promise.reject()
Promise {
  <rejected> undefined,
  domain:
   Domain {
     domain: null,
     _events:
      { removeListener: [Function: updateExceptionCapture],
        newListener: [Function: updateExceptionCapture],
        error: [Function: debugDomainError] },
     _eventsCount: 3,
     _maxListeners: undefined,
     members: [] } }
> (node:28258) UnhandledPromiseRejectionWarning: undefined
(node:28258) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:28258) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
  1. Probably an issue of deasync2. Will try to reproduce and report there

@webmaster128 webmaster128 merged commit 27143a0 into master Jul 20, 2018
@ethanfrey
Copy link
Contributor

ethanfrey commented Jul 20, 2018

Okay, will merge this, works in positive case... what is left to do is make it more robust:

  • Fix wait() hanging on error
  • Fix ts-node exiting on rejected promise.

Simple hack to fix wait issue (need to refine it a bit):

> function safewait<T>(promise: Promise<T>): T { 
  const res = wait(promise.catch(console.log));
  if (!res) { throw new Error('wait failed'); } 
  return res; 
}
undefined
> safewait(adie())
Error: async die
    at [eval].ts:1:11
    at Generator.next (<anonymous>)
    at [eval].ts:6:71
    at new Promise (<anonymous>)
    at __awaiter ([eval].ts:2:12)
    at adie ([eval].ts:1:20)
    at [eval].ts:1:10
    at Script.runInThisContext (vm.js:91:20)
    at exec (/Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:228:17)
    at /Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:218:27
[eval].ts:2
    throw new Error('wait failed');
    ^

Error: wait failed
    at safewait ([eval].ts:2:11)
    at [eval].ts:1:1
    at Script.runInThisContext (vm.js:91:20)
    at exec (/Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:228:17)
    at /Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:218:27
    at Array.reduce (<anonymous>)
    at _eval (/Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:217:18)
    at REPLServer.replEval (/Users/ethan/js/web4/node_modules/ts-node/src/bin.ts:290:14)
    at bound (domain.js:396:14)
    at REPLServer.runBound [as eval] (domain.js:409:12)

(Ideally this "hack" would throw the original error, not print and throw another one)

@webmaster128 webmaster128 deleted the web4write branch July 20, 2018 14:28
@ethanfrey
Copy link
Contributor

  1. unhandled promise rejections are not supposed to be handled. Confirm
    (node:28258) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Fair enough, but we should make it safe when using wait() then, which may expand the scope of the other issue.

Basically wait(Promise.reject()) should throw an Error and not end the process, and if we wrap all Promises with wait, then we should not be at risk for repl termination on error.

@webmaster128
Copy link
Contributor Author

Yeah, I consider the missing reject handling a bug in deasync2.await ("Probably an issue of deasync2. Will try to reproduce and report there"). deasync2.await is supposed to work like the await keyword and must convert rejections into exceptions.

@ethanfrey
Copy link
Contributor

Okay, so we just wait for upstream fix?

@webmaster128
Copy link
Contributor Author

wait for upstream fix

I think I can propose a fix upstream. Should be relatively easy: https://github.com/bluelovers/deasync/blob/master/index.js#L77

@webmaster128
Copy link
Contributor Author

Here you go: bluelovers/deasync#4 I'd wait a couple of days for a merge. But if you think it is critical, we can switch to the fork at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants