fixes #454. Added DisconnectedErrCB to client #462

mkorolyov · 2019-05-14T21:05:26Z

No description provided.

coveralls · 2019-05-14T21:09:39Z

Coverage decreased (-0.1%) to 92.334% when pulling 9a2aafe on mkorolyov:disconnect_err into 4c154eb on nats-io:master.

kozlovic

Thank you for the contribution! I think it looks good, the only comments I would have are about the doc of the disconnected handlers.

kozlovic · 2019-05-14T21:14:02Z

nats.go

@@ -266,8 +269,15 @@ type Options struct {

 	// DisconnectedCB sets the disconnected handler that is called
 	// whenever the connection is disconnected.
+	// Will not be called if DisconnectedErrCB was initiated.


should it be: Will not be called if DisconnectedErrCB is set

thanks, fixed docs

kozlovic · 2019-05-14T21:14:44Z

nats.go

@@ -266,8 +269,15 @@ type Options struct {

 	// DisconnectedCB sets the disconnected handler that is called
 	// whenever the connection is disconnected.
+	// Will not be called if DisconnectedErrCB was initiated.
+	// DEPRECATED. use DisconnectedErrCB which passes initial err


which passes error that caused the disconnect event?

thanks, fixed docs

kozlovic · 2019-05-14T21:15:10Z

nats.go

 	DisconnectedCB ConnHandler

+	// DisconnectedErrCB sets the disconnected error handler that is called
+	// whenever the connection is disconnected.
+	// DisconnectedCB will not be called if DisconnectedErrCB was initiated.


DisconnectedCB will not be called if DisconnectedErrCB is set?

thanks, fixed docs

wallyqs · 2019-05-14T21:20:07Z

nats.go

-			nc.ach.push(func() { nc.Opts.DisconnectedCB(nc) })
+		if nc.conn != nil {
+			if nc.Opts.DisconnectedErrCB != nil {
+				nc.ach.push(func() { nc.Opts.DisconnectedErrCB(nc, ErrConnectionClosed) })


I wonder if maybe here we should use nil for the error instead of ErrConnectionClosed? That would mean that when calling nc.Close(), the disconnected callback is called but there is no error since it was called by the user.

Right. We should then add this to the "doc" of DisconnectedErrCB that it is possible to get nil as an error, one case would be when the user closes the connection.

Hi @wallyqs , thanks for review. Found possible missed case with err passing:
nats.go:1438 where we trying to initialize connected connection and got an err as result, if so we close connection with status DISCONNECTED. I think we should pass acquired error to new callback too, adding err as param to close() method? WDYT?
Adding err as param to close() will fix pointed issue when user closes connection, we will pass nil here with status CLOSED.

I think that sounds good (thanks!)

kozlovic · 2019-05-14T21:23:28Z

nats.go

-			nc.ach.push(func() { nc.Opts.DisconnectedCB(nc) })
+		if nc.conn != nil {
+			if nc.Opts.DisconnectedErrCB != nil {
+				nc.ach.push(func() { nc.Opts.DisconnectedErrCB(nc, ErrConnectionClosed) })


Right. We should then add this to the "doc" of DisconnectedErrCB that it is possible to get nil as an error, one case would be when the user closes the connection.

…ted connection failed to initialize

mkorolyov · 2019-05-15T09:00:03Z

@kozlovic @wallyqs Thanks for fast review! I've updated docs and added logic with proper err passing for close/disconnect status. Looks like it could be merged.

One thing i want to mention that i noticed blinking test TestRecvChanAsyncLeakGoRoutines, but it became green after another travis build. I didn't found any relation between changes i've made and that blinking test, could you pls take a look to make sure we are not breaking something under the hood? Thanks a lot!

kozlovic

Sorry, I would like a second pass on comments. After that I think we can merge. Thanks again for the contribution!

nats.go

kozlovic · 2019-05-16T15:30:05Z

@mkorolyov I was getting ready to do a release and noticed that we missed some deprecated statements, but then I am having second thoughts. I wonder if we should not have named the handler type ConnErrHandler func(*Conn, error) instead of DisconnectErrHandler. The reason is that we could later on use that for other events that disconnect, so I would not want to limit ourselves (or have to add yet another handler). What do you think? I will submit a PR to see how that looks.

mkorolyov · 2019-05-16T15:33:08Z

@mkorolyov I was getting ready to do a release and noticed that we missed some deprecated statements, but then I am having second thoughts. I wonder if we should not have named the handler type ConnErrHandler func(*Conn, error) instead of DisconnectErrHandler. The reason is that we could later on use that for other events that disconnect, so I would not want to limit ourselves (or have to add yet another handler). What do you think? I will submit a PR to see how that looks.

@kozlovic i can rename myself in PR, have some time at the moment :)

kozlovic · 2019-05-16T15:34:39Z

Ok great!

fixes nats-io#454. Added DisconnectedErrCB to client

39ee2f2

kozlovic reviewed May 14, 2019

View reviewed changes

wallyqs reviewed May 14, 2019

View reviewed changes

kozlovic requested changes May 14, 2019

View reviewed changes

Pass nil to new handler if user closes connection. Pass err if connec…

86f1f4b

…ted connection failed to initialize

kozlovic requested changes May 15, 2019

View reviewed changes

nats.go Outdated Show resolved Hide resolved

nats.go Outdated Show resolved Hide resolved

nats.go Outdated Show resolved Hide resolved

fixed docs

9a2aafe

kozlovic approved these changes May 15, 2019

View reviewed changes

kozlovic merged commit 8318b38 into nats-io:master May 15, 2019

mkorolyov deleted the disconnect_err branch May 16, 2019 08:04

This was referenced Oct 5, 2022

Implement Connection DisconnectedErr Callback nats-io/nats.java#764

Closed

Implement Connection DisconnectedErr Callback nats-io/nats.py#363

Open

Implement Connection DisconnectedErr Callback nats-io/nats-pure.rb#84

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #454. Added DisconnectedErrCB to client #462

fixes #454. Added DisconnectedErrCB to client #462

mkorolyov commented May 14, 2019

coveralls commented May 14, 2019 •

edited

Loading

kozlovic left a comment

kozlovic May 14, 2019

mkorolyov May 14, 2019

kozlovic May 14, 2019

mkorolyov May 14, 2019

kozlovic May 14, 2019

mkorolyov May 14, 2019

wallyqs May 14, 2019

kozlovic May 14, 2019

mkorolyov May 14, 2019

wallyqs May 14, 2019 •

edited

Loading

kozlovic May 14, 2019

mkorolyov commented May 15, 2019

kozlovic left a comment

kozlovic commented May 16, 2019

mkorolyov commented May 16, 2019

kozlovic commented May 16, 2019

fixes #454. Added DisconnectedErrCB to client #462

fixes #454. Added DisconnectedErrCB to client #462

Conversation

mkorolyov commented May 14, 2019

coveralls commented May 14, 2019 • edited Loading

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyqs May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkorolyov commented May 15, 2019

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic commented May 16, 2019

mkorolyov commented May 16, 2019

kozlovic commented May 16, 2019

coveralls commented May 14, 2019 •

edited

Loading

wallyqs May 14, 2019 •

edited

Loading