android10 / frodo

Android Library for Logging RxJava Observables and Subscribers.
1.48k stars 101 forks source link

@RxLogSubscriber NullPointerException #14

Closed spirosoik closed 8 years ago

spirosoik commented 8 years ago

There are a few problems with a library dude which I will try to solve and send a PR. I found that if send an empty observable back or a null value back then the RxLogSubscriber crashes with NullPointerException. If I removed it then the subscriber works normally.

android10 commented 8 years ago

@spirosoik great. If you have more details, please put them here so we can find out what the problem is and solve it :)

anpez commented 8 years ago

I have this very same problem. Logging an observable with fixed elements reproduces the problem on the emulator with Android 4.4. On my Nexus 5 with Android 6 works perfectly. Of course, it only happens when compiled for debug. Below the code that reproduces the problem.

@Singleton
public class LocalFrames {
  private static final String TAG = LocalFrames.class.getSimpleName();

  private List<Frame> frames = new ArrayList<>();

  @Inject
  public LocalFrames(AssetManager assetManager) {
    // Insert into "frames" a fixed set of elements, read from assets.
  }

  @RxLogObservable
  public Observable<Frame> all() {
    return Observable.from(frames);
  }
}
android10 commented 8 years ago

@ANPez thanks for reporting this. I have plans to release a new version by the end of next week with some fixes (this one included).

spirosoik commented 8 years ago

@android10 I found the problem. Let's assume that for some reason we want to sent back to the subscriber a null observable Observable.just(null). So Here we are trying to fetch the args of the Subscriber join point where in this case the this.methodParamValuesList will be null.

But the problem is here in the LogSubscriber#beforeOnNextExecution in line 82 method where we are fetching the methodParamValuesList back. As you understand the getMethodParamValuesList() in this case will be null so when we are trying to log the value.toString() a NullPointerException will occur because of value.toString(). value=null.

So let's decide the solution to send a PR. We can do three different things: 1) Here we can check if it's null and instead to set an empty array. eg.

this.joinPoint.getArgs() != null ? Arrays.asList(this.joinPoint.getArgs()) : Collections.emptyList(); 

2) Or we can check here if the value is null and show a proper informative message back 3) Both of them

spirosoik commented 8 years ago

@android10 @ANPez which you prefer guys?

android10 commented 8 years ago

@spirosoik good catch! I think since this is an utility library for logging and debugging, it is nice to be defensive. I would vote for both in this case. If you have plans to open a PR, we can comment and discuss over there, which in my opinion is the best way since we are directly dealing with the code :). What do you think?

spirosoik commented 8 years ago

@android10 I agree i prefer both of them. Yes I will send a PR dude and we can discuss there.

android10 commented 8 years ago

@spirosoik I really appreciate that man! :+1:

spirosoik commented 8 years ago

@android10 Glad to help dude.