TrendrrNSQClient
TrendrrNSQClient copied to clipboard
Fix Null Pointer Exception in NSQConsumer
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)
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:
@dustismo any thoughts?
@kenshiro-o sorry it took so long for a response. I made a couple of comments otherwise looks good, thanks!
@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 Looks good to me. Looks like there is some conflict with master, so if you fix that I will merge.
@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 hi Ed :smile:
I've taken over maintenance of this repo - what do we need to do here to get this in?
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: .