added proxyConnectCb (#870) #871
Conversation
|
for context @wolfkor helped us adding this feature to .NET client nats-io/nats.net#826 I'm assuming this is to implement the same here. I think the idea here has lots of potential even though the main use case now is to punch through (probably corporate) proxies. It sounds good to me in general. I shall leave it to @levb and @kozlovic if it's something they want to consider. |
|
It's a pity that nothing is happening here @kozlovic |
There was a problem hiding this comment.
Sorry for the delay. I am not certain of the intended use-case, so I would need more information about how you plan to use that. Also, we would really need to add some tests (for instance provide a callback that returns an error and verify that connection fails), and one callback that does create socket ok. If you don't know how to add a test, I could add it after merging (if we accept the PR once we resolve the issues). Thanks!
| memset(&hints,0,sizeof(hints)); | ||
| hints.ai_socktype = SOCK_STREAM; | ||
| // Invoke the proxy connect callback. | ||
| ctx->fd = ctx->proxyConnectCb(host, port); |
There was a problem hiding this comment.
I have read in the issue and the work you have done in .NET that you were planning to use that also for TLS. How would that work though? The library is expecting to receive the first INFO protocol in clear text and will then switch to TLS if it is required from the server. In other words, if you are planning to have the library behaves as there is no TLS but have a TLS connection created in this callback, that will fail.
If the callback is solely responsible to create a socket, then maybe ok.
| // Invoke the proxy connect callback. | ||
| ctx->fd = ctx->proxyConnectCb(host, port); | ||
| if (ctx->fd == NATS_SOCK_INVALID) | ||
| s = nats_setError(NATS_SYS_ERROR, "proxy connect socket error: %d", NATS_SOCK_GET_ERROR); |
There was a problem hiding this comment.
Looks like an issue with formatting (tab is 4 spaces).
| } | ||
| else | ||
| { | ||
| if ((ctx->orderIP == 46) || (ctx->orderIP == 64)) |
There was a problem hiding this comment.
The rest is a lot of changes but I assume just to put it under the else without other code modifications, correct? At the same time it look like the case of the switch below has not moved. It could be your preference to be this way, but to stay with the style of the existing code, I would move them too.
At this point, I wonder (again to minimize the changes) if it would not be worth copying the check for total timeout in the "if (hasProxyConnectCb)` and even return from that if. That will leave the rest of the code formatted/indented the way it was.
| @@ -403,6 +403,9 @@ _createConn(natsConnection *nc) | |||
| // Set ctx.noRandomize based on public NoRandomize option. | |||
| nc->sockCtx.noRandomize = nc->opts->noRandomize; | |||
|
|
|||
| // Set the proxy connect callback | |||
There was a problem hiding this comment.
Formatting (again, a tab is 4 spaces)
| /** \brief Callback used to handle connections via proxy. | ||
| * | ||
| * This callback is used to handle proxy connection. It is invoked before the | ||
| * main connections and return the socket that will be used to connect to the server. |
There was a problem hiding this comment.
Not sure what you mean by It is invoked before the main connections...
Also, you don't describe what the user should return if there is an error (unable to create the socket). I don't think NATS_INVALID_SOCKET is exported, so you could say return -1, but we could also change the signature to return natsStatus and have the socket returned through params: (*natsProxyConnHandler)(natsSock **fd, char* host, int port);
| * | ||
| * @param opts the pointer to the #natsOptions object. | ||
| * @param proxyConnHandler the proxy connection handler callback. | ||
| * the callback. `closure` can be `NULL`. |
There was a problem hiding this comment.
Remove this line. But we may want to think if it would be helpful to actually pass a closure.
add proxyConnectCb for TLS connection via proxy (#870)