TrendrrNSQClient icon indicating copy to clipboard operation
TrendrrNSQClient copied to clipboard

Fix Null Pointer Exception in NSQConsumer

Open kenshiro-o opened this issue 10 years ago • 8 comments

The code in the NSQConsumer.java does not check for a null Connection being returned from the call to

super.createConnection(address, port)

This PR fixes this issue which is manifested when we try to call a method on a null Connection (e.g. https://github.com/dustismo/TrendrrNSQClient/blob/master/src/main/java/com/trendrr/nsq/NSQConsumer.java#L39).

Moreover, some refactoring was done and tests added. In case you are interested, the stack trace looked as follows:

        at com.trendrr.nsq.NSQConsumer.createConnection(NSQConsumer.java:40)
        at com.trendrr.nsq.AbstractNSQClient.connect(AbstractNSQClient.java:183)
        at com.trendrr.nsq.AbstractNSQClient$1.run(AbstractNSQClient.java:75)
        at java.util.TimerThread.mainLoop(Timer.java:555)
        at java.util.TimerThread.run(Timer.java:505)

kenshiro-o avatar Apr 15 '14 15:04 kenshiro-o

I will try to find some time in the next few months/weeks to add more tests and improve code. The library works really well otherwise :+1:

kenshiro-o avatar Apr 15 '14 15:04 kenshiro-o

@dustismo any thoughts?

kenshiro-o avatar Apr 24 '14 14:04 kenshiro-o

@kenshiro-o sorry it took so long for a response. I made a couple of comments otherwise looks good, thanks!

dustismo avatar Apr 28 '14 20:04 dustismo

@dustismo I have removed the model package. With regards to changing the scope and name of the Logger across all source files, I'd rather do this in a separate PR to better contain the change.

Also the main reason why I modified your Logger instances is because they should be immutable (final) and also only relevant to the current class. You don't need to use protected as your subclasses define their own logger and hence do not inherit their parent's logger. Lastly the name was capitalized by convention since your Logger is a constant now.

Let me know if you are OK with the current PR :+1:

kenshiro-o avatar May 11 '14 16:05 kenshiro-o

@kenshiro-o Looks good to me. Looks like there is some conflict with master, so if you fix that I will merge.

dustismo avatar Jun 19 '14 22:06 dustismo

@dustismo I have finally found the time to merge the master changes into my branch. Take a look and let me know what you think!

Cheers, Ed.

kenshiro-o avatar Jul 14 '14 09:07 kenshiro-o

@kenshiro-o hi Ed :smile:

I've taken over maintenance of this repo - what do we need to do here to get this in?

mreiferson avatar Feb 25 '15 21:02 mreiferson

Hey @mreiferson :smile_cat:

I had completely forgotten about this PR... I think we just need to check that I have not broken anything :fearful: . But we've actually been running this version at Hailo for a long time so believe it's all good :wink: .

kenshiro-o avatar Feb 26 '15 16:02 kenshiro-o