msemys / esjc

EventStore Java Client
MIT License
108 stars 27 forks source link

Reconnect instead of throwing NPE when connection is null #59

Closed newson-synopsys closed 4 years ago

newson-synopsys commented 4 years ago

For #52

newson-synopsys commented 4 years ago

@msemys @StephanPraetsch I'm still trying to understand the connection lifecycle and the handler pipeline, so this might just be for communicating what I'm thinking about. I'm thinking that the EventStoreTcp should reset back to the reconnecting phase if the connection is dropped, instead of throwing an NPE.

nfode commented 4 years ago

Hi @newson-synopsys. Thanks for your work on this issue. Is your PR ready to be used? I also experienced some problems with the connection being null, therefore I want to use your changes. Thx!

newson-synopsys commented 4 years ago

@nfode this has not been an issue for us recently, so we are not actively working on it. If you can confirm that this change works and fixes your issue though then that would be useful information and might help get this MR accepted! Perhaps if you can write an automated red->green test?

nfode commented 4 years ago

Thank you for your answer, doing further debugging I realized my problem was another one (#62).

ms42 commented 4 years ago

@newson-synopsys We are using this PR since about 1 month and I can confirm that it solved our issue and we haven't experienced other issues so far.

tandr commented 4 years ago

Can we merge this pretty please with a cherry on top? (I really don't want to start whole forking process internally...)

newson-synopsys commented 4 years ago

@msemys what can we add to this PR to make it ready for merge?

msemys commented 4 years ago

that's ok. i was thinking about to throw dedicated exception, but your proposed solution to reconnect when connection is lost while switching to IDENTIFICATION / CONNECTED phase is much better.

thanks :+1: sorry for delay ;)

tandr commented 4 years ago

@msemys -- Thank you! Will you be cutting 2.2.1 at this point, sorry?