0

We have a POJO in our Java application let's say Request which has validateRequest() method that basically validates some of it's fields using Guava's Preconditions library.

So, Basically Class looks like this :

public class Request { 
   private Integer field1; 
   
   private String field2;
   .
   .
   private String fieldN;

   
   public String validateRequest() {
      try { 
           Preconditions.checkArgument( getField1() >= 0, "error message" );

           Preconditions.checkArgument( StringUtils.isNotBlank(getField2()), "error message" );
           .
           .
        } catch (IllegalArgumentException | NullPointerException e){
            //emit some metric
            return e.getMessage();
        }
        return null;
   }
}

We call the below method in all the flows after building the Request POJO.

public void someMethod(Request request) {
    String message = request.validateRequest();
    if(message!= null)
       throw new Exception(message);
}

In some of the flows in our application we are never initialising field1 and field2 , so they are always null in those flows. So, ideal expectation is that validateRequest should return error message for those flows. But, what we notice is production is that this application never returns error message. It is as if Preconditions are never executed! Even metric that we emit is zero. On searching web, found out that compiler's optimisation can cause certain validation/assertion statements to be ignored, but didn't get more details around it.

When we explicitly remote debug to our Production box and put break-point on precondition statements they get executed and error message is returned and metric is emitted.

When we deploy this same application is deployed on Stage, code behaves as expected and throws validation error and metric is emitted.

Want to understand if compiler optimisation is the culprit here, or if root cause could be something else. How to verify that compiler optimisation is causing Preconditions to be ignored?

Pankaj Jahagirdar
  • 21
  • 1
  • 5
  • Is that method even called at all? – Olivier Grégoire Jan 11 '23 at 16:21
  • Yes we call it in all flows. That's why it gets hit during remote debug. – Pankaj Jahagirdar Jan 11 '23 at 17:07
  • Let me rephrase : is that method called when you want it to be called? It could be called at some point, but not in every scenario you're testing. Just because it's a hit when you remote debug it means that it's exactly when you want it. – Olivier Grégoire Jan 11 '23 at 19:09
  • Yes, we call it in all possible flows. What's expected is validation should fail for some flows. – Pankaj Jahagirdar Jan 12 '23 at 02:19
  • 4
    Then the answer is that it CANNOT be optimized away. Your problem is caused by something else. For example, some of your other assumptions are incorrect. It is worth noting that debugging a program can cause various behavior related to memory visibility and concurrency to change. So that's another implicit assumption that could be incorrect. – Stephen C Jan 12 '23 at 04:39
  • Your analysis has a few assumptions — like that this method gets called, that the getters return null, and that the catch isn't being triggered — that a couple log lines could very quickly validate. If you haven't done that yet, it's where I might start. (Just as a guess: you haven't shown the getters, so could it be they're providing a default? And you haven't shown the object's instantiation, so could it be that process is setting defaults?) – yshavit Jan 12 '23 at 04:59
  • -> I am sure catch isn't triggered because Metric isn't emitted; same metric gets emitted when in remote debug exception is triggered. -> Have noticed fields are null and not default during remote debugging as well. – Pankaj Jahagirdar Jan 12 '23 at 05:13
  • 1
    @StephenC let’s not call this an optimization, but *expected, reproducible behavior*. The strange thing here is that the OP says that the actual problem has not been revealed during debugging. – Holger Jun 01 '23 at 12:02
  • @Holger - it was the OP that is asking if this is the result of "optimization" ... not me. – Stephen C Jun 01 '23 at 12:05
  • 1
    @StephenC I know. It’s an interesting case of a questioner’s preoccupation influencing the readers. I think, without it, more people would have noticed that the OP’s “*It is as if Preconditions are never executed*” is actually spot on. – Holger Jun 01 '23 at 12:10
  • Or ... we were just answering the question that he asked :-) – Stephen C Jun 01 '23 at 12:12
  • 1
    @StephenC yes, the good old *xy problem*. Though, in the OP’s defense, this question has enough context to get to the actual problem. It only took a bit longer… – Holger Jun 01 '23 at 12:16

1 Answers1

2

This code seems to be a kind of Cargo Cult Programming. It’s not clear what you expect from the call to Preconditions.checkArgument(…), but whatever it is, it’s not there.

When you call

Preconditions.checkArgument( getField1() >= 0, "error message" );

The expression getField1() >= 0 is evaluated in your method. You’ve already done the check before even calling the checkArgument method. All, the method does, is throwing a new IllegalArgumentException if the argument is false, with the message you’ve provided, i.e.

public static void checkArgument(boolean expression, Object errorMessage) {
    if(!expression) {
        throw new IllegalArgumentException(String.valueOf(errorMessage));
    }
}

which you then catch, to extract the very error message, you have provided yourself.

catch (IllegalArgumentException | NullPointerException e){
    //emit some metric
    return e.getMessage();
}

So, you have created an inefficient way to do the same as

if(getField1() < 0) return "error message";

Since the evaluation of getField1() >= 0 happens in your method, before the checkArgument method is invoked, it will also fail with a NullPointerException instead of ever invoking the checkArgument method when getField1() returned null. This JVM generated NullPointerException will have an implementation specific message or no message at all.

This is not an optimization but just the way method invocations work. First, the arguments are evaluated, then, the method is invoked. If the argument evaluation throws an exception the method is not invoked.

It’s not clear how you could see this working with a getField1() returning null during debugging, as it is impossible to pass null to a method expecting a primitive type boolean. Maybe you are confusing it with the scenario of getField2() returning null.

Your second check uses the expression StringUtils.isNotBlank(getField2()) which handles the null case, as isNotBlank will return false when the argument is null.

You can fix the first check by handling the null case, e.g.

if(getField1() == null || getField1() < 0) return "error message";

or, if you insist on creating and catching an exception, just to extract the very message you have specified yourself,

Preconditions.checkArgument(getField1()!=null && getField1()>=0, "error message");
Holger
  • 285,553
  • 42
  • 434
  • 765