Skip navigation

Developer Community

3 Posts authored by: Trey Carroll

Question: What's worse than code that blows up?

Answer:   Code that blows up silently.

 

There are lots of pithy quotes and proverbs about assumptions, but I have a favorite:

 

     ASSuME: Assumptions make an a** out of you and me.

 

When we make assumptions about things like things like data quality, our code can wind up at a dead-end.

 

For example, if you were tasked with writing a ServiceNow function to find the ID of the first VP above an employee, you might assume that you could use the following:

 

 

// @return string sys_id for CIO
// @param  GlideRecord pointing to a user
function getCioUserId(grItEmployee){

      var manager = grItEmployee.manager;

      if( /VP/.test(manager.title )){
            return manager.sys_id;
      } else {
            return getCioUserId(manager);
      }
}

 

But that means that every employee in the chain must have a manager, or else the function will go into infinite recursion.  That bad assumption (that every ServiceNow user has a manager) is a fatal flaw for this piece of code.

 

Sometimes the code can fail in a foreseeable way.  We can plan for such failures by wrapping our code in a try/catch block;

 

Naive approach (counter example):

 

function getApprover(){
     return null;
}
 var approver = getApprover();
 var fn = approver.first_name;

 

 

 

Notice that this issue causes the script to error out and halts all code execution.   If this were to happen in a Workflow activity, then the activity would turn red and the entire item's Workflow would be dead.

 

But If instead we were to wrap the code in a try/catch block, then we can cause the code to fail gracefully, and even retry using another method.

 

var fn;
var approver;
try{ 
     approver = getApprover();
     fn = approver.first_name;
} catch(e){     
     if(/null/.test( e.message )){ // Error message contains 'null'
           approver = getApproverADifferentWay()
           fn = approver.first_name;
     }
}

 

 

Take another example involving a Regular Expression.   If the execution does not yield any match, then our attempt to retrieve the first match is going to fail.

 

try{ 
     // Put code here in case something goes wrong.
     var re = new RegExp('(bar)');
     var someStringToTest = 'foo_baz';
     var m = re.exec(someStringToTest);
     gs.print(m[1]);
} catch(e){     
     gs.error(e.message);
}

 

Since the regex matches zero times, the assumption that there would be something in the array of matches turned out to be a dangerous one.

 

In this case, we don't have a retry; but the catch block gives us a nice way to quickly know what went wrong by logging the error.

 

I take this so far as to break things up into multiple try / catches within the same function, so that I can identify exactly what I was trying to do when the error was thrown. There is nothing more frustrating that receiving "could not access property 'z' from null" or "'undefined' is not an object" error in a lengthy function.

 

Log: Script x broke when you tried to access a property on something that doesn't exist."

Me:  "Great.  Now all I have to do is figure out which dot (on the myriad of possibilities) is actually trying to dereference an object that is undefined.

 

But if we surround different sections of our code with individual try / catch blocks, our code can actually tell us exactly what went wrong.

 

function x(){       
     var anAppleObject = new Orchard(1).pickAnApple();
     var anOrangeObject = new Orchard(2).pickAnOrange();
     try{
          anAppleObject.bite();
     } catch(e){
          gs.addErrorMessage('Exception throw while biting apple:'+e.message);
     }

     try{
          anOrangeObject.bite();
     } catch(e){
          gs.addErrorMessage('Exception throw while biting orange:'+e.message);
     }
}

 

It's a silly example, but it makes the point.   If one of the objects turns out to be undefined, you'll know which one right away.

 

Wrapping all code in try/catch blocks is a foundational best practice that greatly decreases time to troubleshoot issues.   The practice also seems to facilitate better code by forcing us to consider what could go wrong, and what the right response to an error should be.   In our organization, code that is not wrapped in a try/catch block will not pass a code review.   The payoff to the simple discipline of surrounding all of your code with these try/catch constructs cannot be overstated.   Yes, you can still track down the error without this, but the ability to track down precisely what went wrong quickly is greatly enhanced when you log errors strategically.

 

Once you make the decision to embrace a proactive error-handling stance, there are still some practical considerations.   "How do I embrace this practice without making it too cumbersome?"   Syntax Editor Macros to the rescue.

 

I have 2 Syntax Editor Macros so that quickly insert try/catch blocks into any place where I'm writing a script inside of ServiceNow.

 

Using try/catch is super simple for server-side code:

 

// stry: stands for server-side try
try{ 
     // Put code here in case something goes wrong.
} catch(e){
      if(gs.isInteractive() && gs.hasRole('admin')){
           gs.addInfoMessage('Name of module where error happened:'+ e.message);
      }
     gs.error(e.message);
}

 

It's a little more involved on the client, because we must invoke a GlideAjax call in order to get the message into the system log:

 

// ctry: stands for client-side try
try{ 
     // Put code here in case something goes wrong.
} catch(e){
      if(g_user.hasRole('admin')){
           alert('Name of module where error happened:'+ e.message);
      }
     logError(e.message, '<Script Source Goes Here>');
}

function logError(message, source) {
     var errorUtil = new GlideAjax('GMF_ClientLogUtil');
     errorUtil.addParam('sysparm_name', 'error');
     errorUtil.addParam('sysparm_errorMessage', message);
     errorUtil.addParam('sysparm_errorSource', source);
     errorUtil.getXML();//Add a callback if you want to verify lo
}

 

GMF_ClientLogUtil is a very simple client callable GlideAjax script include:

 

var GMF_ClientLogUtil = Class.create();
GMF_ClientLogUtil.prototype = Object.extendsObject(AbstractAjaxProcessor, {

    //Log the error using server side error logging
    error: function () {
          var errorMessage = this.getParameter('sysparm_errorMessage');
          var errorSource = this.getParameter('sysparm_errorSource');
          gs.error(errorMessage,errorSource);
    },

    info: function(){
          var message = this.getParameter('sysparm_message');          
          gs.info(message);
    },

    warn: function(){
          var message = this.getParameter('sysparm_message');          
          gs.warn(message);
    }
});

 

 

So, having gone to all of this trouble, I find it very frustrating that ServiceNow does not always throw an Exception when it ought to.   I had a piece of code (nicely wrapped in try/catch blocks) silently fail recently.  When I went to the error log I found nothing.  Eventually, I looked at all system entries and found a "warning" that my function had exceeded the maximum capacity for the array data type.  Hmmm...  That seems more like an error to me.

 

I have occasionally bumped into these types of omissions in the platform.   But the one that really bothers me is this: A GlideRecord object's addQuery() method throws no error even when we pass a bad column name.   Given the egregious consequences that can result from such a silent error, I find this really unacceptable.  (I worked on a team where a global ServiceNow project was threatened by a silent column-name typo.)   So the GlideRecord class has a serious deficiency.  We simply must know about errors involving GlideRecords.

 

This need led me to create the u_GlideRecord class (see attachment).

 

 

 

 

// Using u_GlideRecord throws an exception when we pass a bad column name
try{
     var gr = new u_GlideRecord("sc_task");
     gr.addQuery( 'snort_description', 'Complete prescribed work' );  // Should be short not sNort
     gr.setLimit(1);
     gr.query();
     while ( gr.next() ) {
          gs.print( gr.number );
     }
} catch(e){
     gs.print(e.message);
}

 

 

A workflow should not be allowed to start until there is at least one supporting task in a related list.

A Major Incident should not be allowed to close if there is not at least on VIP on the watchlist.

A notification should fire if there is not at least one active user in a group.

 

We often find ourselves given requirements that mean we need to find out if there is at least one record that matches a condition.  But there are several ways to accomplish this.

 

You may have already heard that gs.getRowCount() is inferior due to the fact that it iterates the entire record set in order to count the elements.   But writing the code for the Aggregate is just more verbose, and therefore more effort.

 

Consider

 

function getRowCount(){
     var gr = new GlideRecord("sys_user");
     gr.addQuery("active", "true");
     gr.query();
     if(gr.getRowCount() > 0){
          // Do something awesome
     }
}

 

Versus

 

function getAggregateCount(){
     var gr = new GlideAggregate('sys_user');
     gr.addAggregate('COUNT');
     gr.addQuery('active', 'true');
     gr.query();

     if (gr.next() && gr.getAggregate('COUNT') > 0)  {
          // Do something awesome faster
     }
}

 

How much faster is the aggregate method?

 

howLong(getRowCount);
howLong(getAggregateCount);

function howLong(func){
     var cal1 = Packages.java.util.Calendar.getInstance();
     var startMillis = cal1.getTimeInMillis();

     func.call();

     var cal2 = Packages.java.util.Calendar.getInstance();
     var endMillis = cal2.getTimeInMillis();
     gs.print('Millis for Query:'+(endMillis - startMillis ));
}

 

 

Efficiency 1.png

 

The GlideAggregate gets the job done in almost 1/4 of the time.  Wow.  That's compelling.  We really ought to use the GlideAggregate COUNT over getRowCount().

 

Just watch out for one thing:  You have to call next() in order for getAggregate('COUNT') to work!

 

However, this got me thinking.   By writing these conditions...

 

if ( gs.getRowCount() > 0 ) { }
//OR 
if (gr.next() && gr.getAggregate('COUNT') > 0)  { }

 

We're really on trying to figure out if there is at least one row — right?

 

So, what about counting the rows after limiting the result set to a single row.

 

function getOneRow(){
     var gr = new GlideRecord("sys_user");
     gr.addQuery("active", "true");
     gr.setLimit(1);
     gr.query();
     if(gr.getRowCount() > 0){
          // Do something awesome at blinding speed
     }
}

 

//Drum roll please
howLong(getOneRow);

 

Efficiency 2.png

 

So that's 1/11th of the time that it took without the setLimit(1) statement.   But, surprisingly, it beat the getAggregate('COUNT') method by a factor of 3.

 

Another factor to consider is that for this example performance has been measured using the sys_user table, which has relatively few entries.  Execution time for getRowCount() is going to grow proportionally with the number of results; so performance using getRowCount() with a table with a large number of records is going to be much worse.

 

So the next time I need you need know if there is at least one record that matches a particular condition, try using getRowCount() with a setLimit(1) statement.

 

ctomasi Thanks for challenging me on that sloppy answer.  

 

Now my question to any MSSQL database gurus out there:   What are the database impacts of each of these approaches?    I wouldn't want to count something as a performance win if it was just pushing the work into the database where it was still a performance bottleneck for the instance!

We noticed a very odd issue yesterday when running a "no brainer" script to update items in a GlideRecord while next loop:

 

var gr = new GlideRecord("cmdb_ci");
  gr.addNotNullQuery('u_vulnerability_remediation_group');
  gr.query();

  while ( gr.next() ) {
       try {
            gr.setValue('u_vulnerability_remediation_group',  '');
            gr.update();
       } catch(e) {
            gs.print(e.message);
       }
  }

 

I have used code like this for years in SNOW and it just works (err.. worked).  I have some examples just like this actually checked into source control!

 

Under Helsinki, the actual results of this script were "intriguing":  It deleted a single item and then exited the loop. 

 

After some tinkering I discovered that a function wrapper solved the problem.

 

//After wrapping the code in a IFFE, the loop continues to update items as expected.
(function() {
  var gr = new GlideRecord("cmdb_ci");
  gr.addNotNullQuery('u_vulnerability_remediation_group');
  gr.query();
  while ( gr.next() ) {
       try{
            gr.setValue('u_vulnerability_remediation_group',  '');
            gr.update();
       } catch(e) {
            gs.print(e.message);
       }
  }
}());

Filter Blog

By date: By tag: