buildInvalidMessage hangs, then crashes due to out of memory.

[binarymist]

When buildInvalidMessage returns, execution hangs for about 20 seconds. Then:

<--- Last few GCs --->

[12069:0x2756b60]   159299 ms: Mark-sweep 4562.8 (4611.2) -> 4562.8 (4611.7) MB, 277.2 / 0.0 ms  (+ 1803.3 ms in 7421 steps since start of marking, biggest step 19.2 ms, walltime since start of marking 3997 ms) allocation failure GC in old space requested[12069:0x2756b60]   162828 ms: Mark-sweep 5341.1 (5402.7) -> 2472.0 (2496.4) MB, 510.6 / 0.0 ms  (+ 1002.9 ms in 4154 steps since start of marking, biggest step 18.6 ms, walltime since start of marking 2408 ms) allocation failure GC in old space requested

<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x1197d9525ee1 <JSObject>
    0: builtin exit frame: getOwnPropertyNames(this=0x1197d95046c9 <JSFunction Object (sfi = 0xd561d209d81)>,0xba0f328bb41 <JSArray[75209228]>)

    2: arrayIndexes [/* anonymous */:2] [bytecode=0x1687d8032f91 offset=248](this=0x3a5bef303a21 <JSGlobal Object>,object=0xd561d202351 <the_hole>)
    3: next(this=0x33aff79a00f9 <JSGenerator>)
    5: packRanges [/* anonymous */:4] [bytecode=0x1687d803...

FATAL ERROR: invalid table size Allocation failed - JavaScript heap out of memory
 1: node::Abort() [node]
 2: 0x121a2cc [node]
 3: v8::Utils::ReportOOMFailure(char const*, bool) [node]
 4: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [node]
 5: v8::internal::OrderedHashTable<v8::internal::OrderedHashSet, 1>::Allocate(v8::internal::Isolate*, int, v8::internal::PretenureFlag) [node]
 6: v8::internal::OrderedHashTable<v8::internal::OrderedHashSet, 1>::Rehash(v8::internal::Handle<v8::internal::OrderedHashSet>, int) [node]
 7: v8::internal::OrderedHashTable<v8::internal::OrderedHashSet, 1>::EnsureGrowable(v8::internal::Handle<v8::internal::OrderedHashSet>) [node]
 8: v8::internal::OrderedHashSet::Add(v8::internal::Handle<v8::internal::OrderedHashSet>, v8::internal::Handle<v8::internal::Object>) [node]
 9: v8::internal::KeyAccumulator::AddKey(v8::internal::Handle<v8::internal::Object>, v8::internal::AddKeyConversion) [node]
10: 0xe155e1 [node]
11: v8::internal::KeyAccumulator::CollectOwnElementIndices(v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::Handle<v8::internal::JSObject>) [node]
12: v8::internal::KeyAccumulator::CollectOwnKeys(v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::Handle<v8::internal::JSObject>) [node]
13: v8::internal::KeyAccumulator::CollectKeys(v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::Handle<v8::internal::JSReceiver>) [node]
14: v8::internal::KeyAccumulator::GetKeys(v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::KeyCollectionMode, v8::internal::PropertyFilter, v8::internal::GetKeysConversion, bool) [node]
15: v8::internal::Builtin_ObjectGetOwnPropertyNames(int, v8::internal::Object**, v8::internal::Isolate*) [node]
16: 0x17f74448441d

Also setValue is executed for multiple types.

[binarymist]

Execution stops here after about 20 seconds, and you can see the memory on the left:

You can see the culprit in this below image. If a URL exists in the desc property of the opts of an option, then this happens, if the URL is removed, then it seems OK.

[binarymist]

These are the codes that are broken, can you assist @nexdrew? None of them are commented out when things are not working obviously.

https://github.com/binarymist/mailgun-mate/blob/26afef72c2df6446ae4d773b6ebd285df5811c4e/src/cmds/schedule-delivery.js#L14-L47

class mailgunDateTimeFormat extends Type {
  get datatype () {
    return 'mailgunDateTimeFormat'
  }
  setValue (context, value) {
    
    debugger;
    context.assignValue(this.id, value);
  }
  validateValue (value) {
    debugger;
    // https://momentjs.com/docs/#/parsing/string-format/
    const scheduledTime = moment(value, config.get('mailgunTimeFormat'))
    internals.scheduleDateIsValid = scheduledTime.isValid();
    if (!internals.scheduleDateIsValid) return false;
    internals.scheduleDateIsBeforeDeadline = scheduledTime.isBefore(moment().add(config.get('mailgunMaxFutureScheduleInDays'), 'days'));
    if (!internals.scheduleDateIsBeforeDeadline) return false;
    internals.scheduleDateIsAfterNow = scheduledTime.isAfter(moment());
    if (!internals.scheduleDateIsAfterNow) return false;

    internals.scheduledSendDateTime = value;
    return true;
  }
  buildInvalidMessage (context, msgAndArgs) {
    let customMessage;
    debugger;
    super.buildInvalidMessage(context, msgAndArgs);

    if (!internals.scheduleDateIsValid) customMessage = `The datetime you entered was not valid according to mailgun\'s rules. RFC 2822, formatted as mailgun requires "${config.get('mailgunTimeFormat')}", See (https://documentation.mailgun.com/en/latest/user_manual.html#scheduling-delivery) for more information.`;
    else if (!internals.scheduleDateIsBeforeDeadline) customMessage = `The datetime you entered was outside of the mailgun ${config.get('mailgunMaxFutureScheduleInDays')} days schedule linmmit.`;
    else if (!internals.scheduleDateIsAfterNow) customMessage = `The datetime you entered was in the past.`;
    msgAndArgs.msg += ` Please specify a valid schedule datetime. ${customMessage} Please try again.`
  }
}

https://github.com/binarymist/mailgun-mate/blob/26afef72c2df6446ae4d773b6ebd285df5811c4e/src/cmds/schedule-delivery.js#L319

.registerFactory('mailgunDateTimeFormat', opts => new mailgunDateTimeFormat(opts))

https://github.com/binarymist/mailgun-mate/blob/26afef72c2df6446ae4d773b6ebd285df5811c4e/src/cmds/schedule-delivery.js#L357-L402

  .command('list', {
    desc: 'List members in order based on latest or oldest mailgunMateScheduledSends datetimes.',
    paramsDesc: 'The order to list the items in: "des" for descending, "asc" for ascending.',
    setup: (sywac) => {
      sywac
      .option(
        '-l, --email-list <email-list>',
        {
          type: 'string', desc: 'The mailgun email list you would like to use.', strinct: true, defaultValue: config.get('emailList')
        }
      )
      .option(   
        '-o, --order [des|asc(default)]',
        {
          type: 'string', desc: 'Which order would you like the items displayed in.', defaultValue: config.get('displayOrderOfListMemberScheduledSends')
        }
      );
    },
    run: async (parsedArgv, context) => {
      const argv = parsedArgv;
      debugger;
      internals.listMemberDispalyOrder = argv.o;
      if (parsedArgv.l) {
        internals.mailList = parsedArgv.l;  
      } else {
        return context.cliMessage('You must provide a valid mailgun mail list.');
      }
      internals.mgDomain = config.get('domain');
      console.log(`Your currently configured mailgun domain is "${internals.mgDomain}".`);
      console.log(`Your currently configured mailgun list is "${internals.mailList}".`);
      await authenticateToMailgun();
      debugger;
      await displaySubscribedListMembers();
      
      argv.handled = true;
    }
  })

I also can't get any help working, even with the uncommented following section. What could be causing this?

https://github.com/binarymist/mailgun-mate/blob/26afef72c2df6446ae4d773b6ebd285df5811c4e/src/cmds/schedule-delivery.js#L403-L423

.command('*', {
    desc: 'Default command for schedule-delivery.',
    setup: (sywac) => {
      debugger;
      sywac.help('-h, --help').showHelpByDefault().parseAndExit();
    },
    run: (parsedArgv, context) => {
      debugger
      const argv = parsedArgv;
      if (context.args.length > 1) context.cliMessage(`Unknown argument: ${context.args[1]}`);
 
      context.helpRequested = true;
      
      return argv;
      //argv.handled = true;
    }
  }) ;

The top level help does work.

[nexdrew]

Hi, sorry for the radio silence.

One thing I see is that you're calling parseAndExit() within the setup of your default command. You shouldn't do this. In general, parse or parseAndExit should only be called once for any given CLI execution.

If I get some time I'll try to look a bit further into your code.

[nexdrew]

Also note that if you apply configuration like .help('-h, --help') or .showHelpByDefault() at the top level, you shouldn't apply it again at lower levels (commands or subcommands) because it will be automatically "inherited"/applied at lower levels.

[binarymist]

Yeah, I thought that was the case with both of those, but in order to get it to work, one starts hacking. I think I have bigger problems. Unless Lines 357-421 above are commented out, the schedule-delivery command won't run.

[binarymist]

Any more tips @nexdrew?

[binarymist]

bump

[nexdrew]

@binarymist Apologies, I hadn't given this issue much attention, assuming it was caused by a problem in your code rather than in sywac. But I finally took the time to look into the examples you gave, and I was able to reproduce a bug that showed there was the potential for an infinite loop when generating the help text. See PR #23.

What I'd like to do is merge and publish the fix for that problem and then have you retry your code after upgrading sywac to the latest version. If that fixes things for you, then we can close this issue at that time. Hopefully we don't find other issues. 🤞

For future reference, it would help tremendously if you could provide me with a succinct failing test case. It was not easy to pick out which parts of your code snippets were actually relevant. Your debugger screenshots were a big help though. I appreciate you reporting this issue and sending me a bunch of details, but you might have to help me narrow the focus on what the actual problem/failing use-case is. Thanks.

[nexdrew]

@binarymist Please give sywac@1.2.1 a try and let me know how it goes. Thanks!

[binarymist]

I'm unable to reproduce the:

When buildInvalidMessage returns, execution hangs for about 20 seconds.

I've made a lot of changes since this was reported. Also tested your update and no hang,

Re:

It was not easy to pick out which parts of your code snippets were actually relevant.

You weren't the only one. I thought the very first line of my first comment summed it up:

When buildInvalidMessage returns, execution hangs for about 20 seconds. Then:

Although I'm unable to repro the hang, I can't confirm the fix (sadly), but can neither confirm the hang (kind of happily)

The second two pieces of code are still not working though, that's the command's, if I don't have them commented out, the schedule-delivery doesn't work at all, I just get the help text printed. No errors.

I don't think I'm doing anything wrong, but it's a little tricky to know for sure, as the docs are still fairly sparse, and I've been trying to piece together what sywac expects based on the existing docs and docs from yargs, and varius yargs issues that mostly you have commented on.

Is my nesting somehow not what sywac expects?

[binarymist]

Strangly when I try swapping command for flags as you suggested here I get the error:

.flags is not a function

I'm starting to think that I can not nest commands as seen from L296-L360 like:

schedule-delivery list -l <email-list> -o des

As well as:

schedule-delivery -b <email-body-file> -f <sent-from-for-replies>

Can you confirm @nexdrew that this should or not be possible? Usually the prefered option is to answer these sorts of questions by adding to the documentation, thus hopefully those in the future will not try and implement something that is not supposed to be possible at this stage, if that is in-fact a fact.
Let me know if I can help with this?

[binarymist]

Can you confirm whether or not the mentioned nesting is supported @nexdrew? I thought based on your comments here that it was.

Or maybe I've just missinterpreted something. Can you please confirm either way?

[nexdrew]

Sywac supports nested commands. I can provide you an example tomorrow. In the meantime, could you perhaps give me a simplified version of code you have tried that doesn't work as expected, or maybe point me to a failing example?

[nexdrew]

The way you should declare a nested command is by calling .command() or .commandDirectory() from within the setup handler of the parent command.

[binarymist]

Ok, so I must be doing something wrong (in the non sywac specific code). I'll create a branch and start ripping code out until either it starts to work, or I have that much simplified version for you.

Don't spend any time on it just yet @nexdrew, if it should be working, then it's probably something I'm doing wrong, Let me do this and get back to you.

Much appreciated!

[nexdrew]

No worries! I'd still like to give you a small working example of nested commands for your reference, but I am AFK right now. I should be able to do this tomorrow morning.

[binarymist]

Ideally throw it in the existing documentaion that you've already so lovingly crafted.

[nexdrew]

@binarymist I created a gist with two files that demonstrates a command module with a nested command, based off of the code you referenced above.

https://gist.github.com/nexdrew/6c3598ea534aa6788ff1c11d4e3eade5

The code works as I would expect it to. Here are results of sample execution:

$ node issue21.js
Usage: issue21 <command> [options]

Commands:
  schedule-delivery  Launch scheduled mail delivery, max of three days in advance.

Options:
  -h, --help  Show help                                                   [commands: help] [boolean]

$ node issue21.js schedule-delivery --help
Usage: issue21 schedule-delivery <command> [options]

Commands:
  list  List members in order based on latest or oldest mailgunMateScheduledSends datetimes.

Options:
  -l, --email-list <email-list>                          The mailgun email list you would like to
                                                         use.
                                                         [string] [default: something]

  -b, --email-body-file <email-body-file>                File containing the html for the body of
                                                         the email. Relative to the
                                                         emailBodyFileDir directory you set in the
                                                         configuration.
                                                         [file]

  -f, --from <sent-from-for-replies>                     The value that the receiver will see that
                                                         your emails appear to be sent from, in
                                                         the form of "Kim
                                                         <services@binarymist.net>"
                                                         [string]

  -s, --subject <subject-for-email>                      The subject for the email
                                                         [string]

  -t, --schedule-time <time-to-schedule-email-send-for>  The time that all emails will be sent (in
                                                         RFC 2822 time).
                                                         [mailgunDateTimeFormat]

  -tm, --test-mode                                       Whether or not to send in test mode
                                                         "o:testmode".
                                                         [boolean]

  -h, --help                                             Show help
                                                         [commands: help] [boolean]

$ node issue21.js schedule-delivery list -h
Usage: issue21 schedule-delivery list [options]

Options:
  -o, --order [des|asc(default)]                         The order you would like the items
                                                         displayed in.
                                                         [string] [default: asc]

  -l, --email-list <email-list>                          The mailgun email list you would like to
                                                         use.
                                                         [string] [default: something]

  -b, --email-body-file <email-body-file>                File containing the html for the body of
                                                         the email. Relative to the
                                                         emailBodyFileDir directory you set in the
                                                         configuration.
                                                         [file]

  -f, --from <sent-from-for-replies>                     The value that the receiver will see that
                                                         your emails appear to be sent from, in
                                                         the form of "Kim
                                                         <services@binarymist.net>"
                                                         [string]

  -s, --subject <subject-for-email>                      The subject for the email
                                                         [string]

  -t, --schedule-time <time-to-schedule-email-send-for>  The time that all emails will be sent (in
                                                         RFC 2822 time).
                                                         [mailgunDateTimeFormat]

  -tm, --test-mode                                       Whether or not to send in test mode
                                                         "o:testmode".
                                                         [boolean]

  -h, --help                                             Show help
                                                         [commands: help] [boolean]

$ node issue21.js schedule-delivery -t bad
Usage: issue21 schedule-delivery <command> [options]

Commands:
  list  List members in order based on latest or oldest mailgunMateScheduledSends datetimes.

Options:
  -l, --email-list <email-list>                          The mailgun email list you would like to
                                                         use.
                                                         [string] [default: something]

  -b, --email-body-file <email-body-file>                File containing the html for the body of
                                                         the email. Relative to the
                                                         emailBodyFileDir directory you set in the
                                                         configuration.
                                                         [file]

  -f, --from <sent-from-for-replies>                     The value that the receiver will see that
                                                         your emails appear to be sent from, in
                                                         the form of "Kim
                                                         <services@binarymist.net>"
                                                         [string]

  -s, --subject <subject-for-email>                      The subject for the email
                                                         [string]

  -t, --schedule-time <time-to-schedule-email-send-for>  The time that all emails will be sent (in
                                                         RFC 2822 time).
                                                         [mailgunDateTimeFormat]

  -tm, --test-mode                                       Whether or not to send in test mode
                                                         "o:testmode".
                                                         [boolean]

  -h, --help                                             Show help
                                                         [commands: help] [boolean]

Value "bad" is invalid for argument t or schedule-time. Please specify a valid schedule datetime. The datetime you entered was not valid according to mailgun's rules. RFC 2822, formatted as mailgun requires "YYYY-MM-DD", See (https://documentation.mailgun.com/en/latest/user_manual.html#scheduling-delivery) for more information. Please try again.

$ node issue21.js schedule-delivery
schedule-delivery: { _: [],
  h: false,
  help: false,
  l: 'something',
  'email-list': 'something',
  b: undefined,
  'email-body-file': undefined,
  f: undefined,
  from: undefined,
  s: undefined,
  subject: undefined,
  t: undefined,
  'schedule-time': undefined,
  tm: false,
  'test-mode': false }

$ node issue21.js schedule-delivery list
list: { _: [],
  h: false,
  help: false,
  l: 'something',
  'email-list': 'something',
  b: undefined,
  'email-body-file': undefined,
  f: undefined,
  from: undefined,
  s: undefined,
  subject: undefined,
  t: undefined,
  'schedule-time': undefined,
  tm: false,
  'test-mode': false,
  o: 'asc',
  order: 'asc' }

$ node issue21.js schedule-delivery list -o desc
list: { o: 'desc',
  _: [],
  h: false,
  help: false,
  l: 'something',
  'email-list': 'something',
  b: undefined,
  'email-body-file': undefined,
  f: undefined,
  from: undefined,
  s: undefined,
  subject: undefined,
  t: undefined,
  'schedule-time': undefined,
  tm: false,
  'test-mode': false,
  order: 'desc' }

Please note the inline comments in the gist code.

Also, here are some things I wanted to mention:

• The paramsDesc property for a command is only used if the command has positional arguments in its flags, e.g. 'list [order=asc]' (in which case you could supplant the "-o, --order" option for the list command).

• You have misspelled the "strict" property as "strinct" in a couple places.

• The strict property tells sywac to validate the value given for an option (i.e. call validateValue(value) for that type) and return an error if validation fails. For the built-in sywac types, strict only applies to number. Note that enum types are implicitly strict, and there is a mustExist property that applies for path, dir, or file option types, which is a more specific form of strictness.

• If you want to mark an option as required, then use the required property for an option rather than the strict property. If an option is both required and has a default value, then the default value will never be used.

• In general, options and api configuration applied at one level also applies to all sub-levels (i.e. all subcommands). Therefore, if you define an option for the "schedule-delivery" command, then you don't need to define it again for the "list" subcommand. Any required options for a parent command are also required for any subcommands.

• If you have a parent command with a run handler, then you should disable showing help by default (via .showHelpByDefault(false)) as part of its setup configuration.

• If a subcommand is executed (i.e. its run handler is called), then the run handler for a parent command will not be executed.

[binarymist]

Thanks muchly for that @nexdrew, some crucial points in there that I had missed or wasn't aware of.

I didn't realise strict only applied to number, did I miss this in the docs, or is it not there?

In general, options and api configuration applied at one level also applies to all sub-levels (i.e. all subcommands). Therefore, if you define an option for the "schedule-delivery" command, then you don't need to define it again for the "list" subcommand. Any required options for a parent command are also required for any subcommands.

I wasn't aware of this, again, did I fail to read this in the docs or does it not exist, or maybe needs clearer explanation? After realising this, I realised my list sub command shoudln't have been a sub command after all, as list doesn't require the options of the parent. So I've made it it's own command.

I also replaced the optional order argument with a positional as you suggested.

[binarymist]

Any feedback @nexdrew

[nexdrew]

Docs are definitely lacking at the moment. Otherwise, can we close this issue?

[binarymist]

Sounds like it. Thanks @nexdrew . Only issue I have outstanding now is #25