[FIX] #145 - when using TLS reconnects would fail because of listener leaks (events firing on leaked streams) #147
Conversation
|
Note that I have included 'authorization timeout' as a non-fatal error. Other clients don't handle this. We can remove this behaviour or enhance other clients to reconnect if a server fails to authorize because of some internal load issue. Not sure if this is the right thing to do. |
aricart
changed the title
|
The server should be expected to disconnect after an authorization timeout. I would cleanup the connection and fire the event callbacks. |
|
It does disconnect. Now the question is do we fire 'error'. On other clients that is what would happen. |
|
Yes I believe so.. |
aricart
force-pushed
the
tls-reconnect-leaks
branch
from
9d9dfde
to
9985164
Compare
|
Updated so the client disconnects on the authorization timeout error. Added another test without tls which will always get the error from the server. |
lib/nats.js
Outdated
| @@ -91,6 +91,9 @@ var VERSION = '0.7.18', | |||
| NATS_PROTOCOL_ERR = 'NATS_PROTOCOL_ERR', | |||
| REQ_TIMEOUT = 'REQ_TIMEOUT', | |||
| REQ_TIMEOUT_MSG_PREFIX = 'The request timed out for subscription id: ', | |||
| STALE_CONNECTION_ERR = "stale connection", | |||
| PERMISSIONS_ERR = "permissions violation", | |||
| AUTHORIZATION_TIMEOUT_ERR = "authorization timeout", | |||
test/perms.js
Outdated
| nsc.stop_server(server); | ||
| }); | ||
|
|
||
| it('foo can subscribe/pub foo', function (done) { |
There was a problem hiding this comment.
I would not test the positive cases here, this is a server test, not a client. You want to test a part of the code that handles the failure, so I would keep only negative tests where we try to pub/sub and get an error and check that connection is not closed.
…leaks (events firing on leaked streams)
[FIX] node client fired error on any protocol error. Some protocol errors such as 'stale connection' should have initiated a reconnect.
[FIX] if a client attempted to publish/subscribe to a subject for which it had no permission, an error was generated. New event ('permission_error') will now be sent to the client.
Version Bumps
aricart
force-pushed
the
tls-reconnect-leaks
branch
from
3b1ebf9
to
be8fa84
Compare
1 similar comment
|
LGTM |
[FIX] node client fired error on any protocol error. Some protocol errors such as 'stale connection' should have initiated a reconnect.
[FIX] if a client attempted to publish/subscribe to a subject for which it had no permission, an error was generated. New event ('permission_error') will now be sent to the client.
@Blazedd effectively submitted a similar PR.