Port tokenHandler mechanism from go-nats (option createConnection())

[andriy-bulynko]

Fixes: #265

Description

I wasn't sure of the best place to plug this in. This PR introduces the code into createConnection(). It might work well there because it wouldn't be "interrupting" an otherwise synchronous flow inside the processInbound(). However, it moves the call to obtain the token a bit further from where it would be used. This solution also assumes that the auth is only ever done on a fresh TCP connection. Is this assumption correct?

[coveralls]

Coverage increased (+0.1%) to 89.355% when pulling c21fc5b on andriy-bulynko:token-handler-createConnection into e5f4558 on nats-io:master.

[andriy-bulynko]

The Travis CI seems to be failing on unrelated tests. Is there any way to re-run it?

[aricart]

We don't use Promises in node-nats. Things that require loading information do so in a synchronous mechanism. If your app needs to update the token, likely this should be some function that periodically updates it so you have it available when connect requires it.

[aricart]

Also sendConnect is where authentication is gathered and filled in.

[andriy-bulynko]

@aricart: How firm are you on the "no-async" policy? It certainly makes it more difficult to track the token expiry outside of the lib.

[aricart]

the point is that by the time you get a connect if you don't have the token already resolved, you'll be unceremoniously disconnected. I think you should periodically check for your token or have some other mechanism where you are notified when the token is updated, but that is not for the nats library to resolve. The library should simply be concerned with reading the value as an immediate operation.

If you look at the other resolutions (for creds files) they are immediately resolved. The process charged with updating can do so whenever. The only contract is for the file to be current when we need it.

[aricart]

Also if you are dabbling in Promises or async/await or are looking for more 'modern' javascript, take a look at ts-nats. The contract above would still be true (the limitation is not on client, but that nats servers typically disconnect clients very very quickly if the connect protocol is not fulfilled quickly enough.

[andriy-bulynko]

@aricart: I agree that speed is off the essence. I hope you noticed that in my tests I setup very long timeouts for token resolutions (I think I tried as far as just under 1000ms), and the server was fine. Again, in go-nats the synchronous callbacks can do "anything" synchronously for a period of time, and that period may be non-trivial. In our case, tokenHandler() callbacks in go-nats go out and refreshes tokens. None of that is affecting the timing for the protocol. And even if any individual call would take too long to mess up timing, then there should be a predictable failure followed by a reconnect if configured. Wouldn't you agree?

[andriy-bulynko]

@aricart: Simplicity is nice and how can you get simpler than asking for a token when you need it? Of course, other approaches can be used, but none of them would be as lean as that simple pattern.

[aricart]

@andriy-bulynko I am ok with the asking for the token dynamically, I just don't want you to resolve the token within the sendConnect. By making it a Promise the door is open, because now until it resolves I cannot really attempt a connection. This can easily translate to support issues.

When the resolution of the token is out of band with the client the worse that could happen is that your client will get an authentication error.

[andriy-bulynko]

@aricart: Also, I'm not hung up on promises or any other "modern" ES6 features. I chose the promise approach because it's an easy-to-use data type available in the version of node that's listed as minimally supported by the library. It offers an easy abstraction for returning a value or an error. I could re-write this using the callback(val, error) vanilla function, if you'd prefer that.

[andriy-bulynko]

@aricart: Have you seen the other PR: #266 ? There, I'm attempting to do the same in the processInbound() instead of the sendConnect(). That seems to work well too.

[aricart]

replaced with the sync version