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 |
There was a problem hiding this comment.
this one is still reporting the original uris
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); |
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. |
No need to over optimize redacted value it's never on hot path. Correcness first.
|
hey @tormoder it looks like the problem was the NatsUri redaction caching. it's never on a hot path so just removed the caching of that string. That seems to be fixing the issue, let me know it works for you too. |
Yes, thank you. |
Changes in this release: * Fix StreamSourceInfo.Active handle -1 correctly (#1058) * Fix NatsUri not reflecting host/port changes from OnConnectingAsync hook (#997) Chanfes from preview.2 * Fix OTel activity leak in Direct request mode (#1047) * Fix subject UTF-8 encoding (#1054) * Add SlowConsumerDetected event and warnings (#1052) * Fixed unobserved exceptions surfacing due to unobserved tcs. (#1051) Changes from preview.1 * Fix and improve build for docs ci error (#1040) * resolves: 1035 - Use ConnectTimeout when awaiting INFO signal (#1039) * Fix slow consumer blocking (#1041)
Changes in this release: * Fix StreamSourceInfo.Active handle -1 correctly (#1058) * Fix NatsUri not reflecting host/port changes from OnConnectingAsync hook (#997) Chanfes from preview.2 * Fix OTel activity leak in Direct request mode (#1047) * Fix subject UTF-8 encoding (#1054) * Add SlowConsumerDetected event and warnings (#1052) * Fixed unobserved exceptions surfacing due to unobserved tcs. (#1051) Changes from preview.1 * Fix and improve build for docs ci error (#1040) * resolves: 1035 - Use ConnectTimeout when awaiting INFO signal (#1039) * Fix slow consumer blocking (#1041)
3 participants
Fixes #996.