Fix NatsUri not reflecting host/port changes from OnConnectingAsync hook #997
Conversation
|
Thanks @tormoder good catch! this is fixing some of the logging but exceptions are still coming through with the original URI since the fix is done inside |
|
|
||
| if (_socketConnection == null) | ||
| { | ||
| var exception = new NatsException("can not connect uris: " + string.Join(",", uris.Select(x => x.ToString()))); |
There was a problem hiding this comment.
this one is still reporting the original uris
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(NatsLogEvents.Connection, ex, "Fail to connect NATS {Url}", uri); |
There was a problem hiding this comment.
this is still reporting the original
| } | ||
|
|
||
| var connectionFactory = Opts.SocketConnectionFactory ?? (uri.IsWebSocket ? WebSocketFactory.Default : TcpFactory.Default); | ||
| _logger.LogInformation(NatsLogEvents.Connection, "Connect to NATS using {FactoryType} {Uri}", connectionFactory.GetType().Name, uri); |
| } | ||
| } | ||
|
|
||
| private async Task ConnectSocketAsync(NatsUri uri) |
There was a problem hiding this comment.
maybe this could return the uri (actually that won't work with exception!)
| var target = (uri.Host, uri.Port); | ||
| if (OnConnectingAsync != null) | ||
| { | ||
| _logger.LogInformation(NatsLogEvents.Connection, "Invoke OnConnectingAsync before connecting to NATS {Uri}", uri); | ||
| target = await OnConnectingAsync(target).ConfigureAwait(false); | ||
| if (target.Host != uri.Host || target.Port != uri.Port) | ||
| { | ||
| uri = uri with { Uri = new UriBuilder(uri.Uri) { Host = target.Host, Port = target.Port, }.Uri }; | ||
| var modifiedUri = new UriBuilder(uri.Uri) { Host = target.Host, Port = target.Port }.Uri; | ||
| uri = new NatsUri(modifiedUri.ToString(), uri.IsSeed, uri.Uri.Scheme); | ||
| } | ||
| } |
There was a problem hiding this comment.
maybe this can be extracted into a method?
|
Thanks, I will take another look. |
|
A bit fiddly, but did another attempt. |
thanks @tormoder yes it looks a bit fiddly :) it seems to work though. I will have a closer look once the other PR is merged that fixes the tests as well. |
2 participants
Fixes #996.