Copilot commented on code in PR #2656: URL: https://github.com/apache/plc4x/pull/2656#discussion_r3655399932
########## plc4net/api/api/ConnectionString.cs: ########## @@ -0,0 +1,184 @@ +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +using System; +using System.Collections.Generic; +using System.Text.RegularExpressions; +using org.apache.plc4net.exceptions; + +namespace org.apache.plc4net.api +{ + /// <summary> + /// A parsed PLC4X connection string: + /// <c>{protocol-code}(:{transport-code})?://{transport-config}(?{parameter-string})?</c> + /// </summary> + /// <remarks> + /// The grammar is kept identical to the Java SPI3 <c>DriverBase.URI_PATTERN</c> so the + /// same connection string addresses the same device from either language. Examples: + /// <c>s7://192.168.0.1</c>, <c>s7:cotp://10.0.0.5:102?remote-rack=0&remote-slot=1</c>, + /// <c>modbus-tcp://10.0.0.9:502?unit-identifier=1</c>. + /// </remarks> + public sealed class ConnectionString + { + private static readonly Regex UriPattern = new Regex( + @"^(?<protocolCode>[a-z0-9\-]*)(:(?<transportCode>[a-z0-9\-]*))?://(?<transportConfig>[^?]*)(\?(?<paramString>.*))?$", + RegexOptions.Compiled); + + // Masks the value of any query parameter whose name looks like it carries a + // credential, so connection strings can be logged safely. + private static readonly Regex SecretParamPattern = new Regex( + @"([?&][^=&]*(?:password|passwd|secret|token)[^=&]*=)[^&]*", + RegexOptions.Compiled | RegexOptions.IgnoreCase); + + private ConnectionString( + string protocolCode, + string transportCode, + string transportConfig, + string paramString, + IReadOnlyDictionary<string, string> parameters) + { + ProtocolCode = protocolCode; + TransportCode = transportCode; + TransportConfig = transportConfig; + ParamString = paramString; + Parameters = parameters; + } + + /// <summary>e.g. "s7", "modbus-tcp".</summary> + public string ProtocolCode { get; } + + /// <summary>e.g. "tcp", "cotp". Null when the string relies on the driver's default.</summary> + public string TransportCode { get; } + + /// <summary>The address the transport consumes, e.g. "192.168.0.1:102".</summary> + public string TransportConfig { get; } + + /// <summary>The raw query string, without the leading '?'. Empty when absent.</summary> + public string ParamString { get; } + + /// <summary>Query parameters, parsed. Keys are case-insensitive.</summary> + public IReadOnlyDictionary<string, string> Parameters { get; } + + public static ConnectionString Parse(string connectionString) + { + if (string.IsNullOrWhiteSpace(connectionString)) + { + throw new PlcConnectionException("Connection string must not be empty."); + } + + var match = UriPattern.Match(connectionString); + if (!match.Success) + { + throw new PlcConnectionException( + "Connection string doesn't match the format " + + "'{protocol-code}(:{transport-code})?://{transport-config}(?{parameter-string})?'"); + } + + var protocolCode = match.Groups["protocolCode"].Value; + if (string.IsNullOrEmpty(protocolCode)) + { + throw new PlcConnectionException("Connection string is missing the protocol code."); + } + + var transportGroup = match.Groups["transportCode"]; + var transportCode = transportGroup.Success && transportGroup.Value.Length > 0 + ? transportGroup.Value + : null; + + var paramString = match.Groups["paramString"].Success + ? match.Groups["paramString"].Value + : string.Empty; + + return new ConnectionString( + protocolCode, + transportCode, + match.Groups["transportConfig"].Value, + paramString, + ParseParameters(paramString)); + } + + private static IReadOnlyDictionary<string, string> ParseParameters(string paramString) + { + var result = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); + if (string.IsNullOrEmpty(paramString)) + { + return result; + } + + foreach (var pair in paramString.Split('&')) + { + if (pair.Length == 0) + { + continue; + } + var separator = pair.IndexOf('='); + if (separator < 0) + { + // A bare flag, e.g. "?verbose" — treat as present-and-true. + result[Uri.UnescapeDataString(pair)] = "true"; + continue; + } + var key = Uri.UnescapeDataString(pair.Substring(0, separator)); + var value = Uri.UnescapeDataString(pair.Substring(separator + 1)); + result[key] = value; + } Review Comment: Parameter decoding uses Uri.UnescapeDataString, which does not translate '+' into spaces. Java’s SPI3 configuration parsing URL-decodes parameter values (URLDecoder.decode(..., UTF_8)), so a connection string containing application/x-www-form-urlencoded style spaces (e.g. "name=a+b") will be interpreted differently in plc4net vs plc4j. ########## plc4net/transports/tcp/TcpTransportInstance.cs: ########## @@ -0,0 +1,336 @@ +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +using System; +using System.Net; +using System.Net.Sockets; +using System.Threading; +using System.Threading.Tasks; +using org.apache.plc4net.spi.transports; + +namespace org.apache.plc4net.transports.tcp +{ + /// <summary> + /// TCP transport: one background read loop per connection feeding a ring buffer. + /// </summary> + /// <remarks> + /// The Java SPI3 transport uses a virtual thread per connection blocked in a socket + /// read. .NET has no virtual threads, but async I/O gives the same result: the read + /// loop awaits <c>ReceiveAsync</c> and holds no thread while idle. The observable + /// contract is the same — the loop fills the ring buffer and then invokes the data + /// listener. + /// </remarks> + public class TcpTransportInstance : BaseTransportInstance, IAsyncTransportInstance + { + private static readonly byte[] EmptyBytes = new byte[0]; + + private readonly Socket _socket; + private readonly RingBuffer _ringBuffer; + private readonly object _readLock = new object(); + private readonly object _writeLock = new object(); + private readonly CancellationTokenSource _shutdown = new CancellationTokenSource(); + private readonly Task _readLoop; + + private int _open = 1; + private volatile Action _dataListener; + private volatile Action<Exception> _disconnectListener; + + public TcpTransportInstance(IPEndPoint remoteAddress, TcpTransportConfiguration configuration) + : base(configuration) + { + if (remoteAddress == null) + { + throw new ArgumentNullException(nameof(remoteAddress)); + } + + _ringBuffer = new RingBuffer(configuration.ReceiveBufferSize); + + Socket socket = null; + try + { + socket = new Socket(remoteAddress.AddressFamily, SocketType.Stream, ProtocolType.Tcp); + + if (!string.IsNullOrEmpty(configuration.LocalAddress)) + { + socket.Bind(new IPEndPoint(IPAddress.Parse(configuration.LocalAddress), configuration.LocalPort)); + } + + socket.NoDelay = configuration.TcpNoDelay; + socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, configuration.KeepAlive); + if (configuration.SendBufferSize > 0) + { + socket.SendBufferSize = configuration.SendBufferSize; + } + if (configuration.ReceiveBufferSize > 0) + { + socket.ReceiveBufferSize = configuration.ReceiveBufferSize; + } + + Connect(socket, remoteAddress, configuration.ConnectTimeout); + + _socket = socket; + RemoteAddress = remoteAddress; + LocalAddress = socket.LocalEndPoint as IPEndPoint; + } + catch (Exception e) + { + socket?.Dispose(); + throw new TransportException( + $"Failed to connect to {remoteAddress.Address}:{remoteAddress.Port} - {e.Message}", e); + } + + // Started last so a throw during setup cannot leak a running loop. + _readLoop = Task.Run(RunReadLoopAsync); + } + + public IPEndPoint RemoteAddress { get; } + + public IPEndPoint LocalAddress { get; } + + /// <summary> + /// Connects with a bounded wait. ConnectAsync has no timeout overload, so the + /// timeout is applied by racing it and disposing the socket on expiry — closing + /// the socket is what aborts an in-flight connect. + /// </summary> + private static void Connect(Socket socket, IPEndPoint remoteAddress, int connectTimeoutMillis) + { + var connectTask = socket.ConnectAsync(remoteAddress); + if (!connectTask.Wait(connectTimeoutMillis)) + { + throw new TimeoutException( + $"Connection to {remoteAddress.Address}:{remoteAddress.Port} timed out after {connectTimeoutMillis} ms."); + } + // Surface any connect error (Wait already completed, so this cannot block). + connectTask.GetAwaiter().GetResult(); + } + + public override bool IsOpen => Volatile.Read(ref _open) == 1 && _socket.Connected; + + public override int GetNumBytesAvailable() + { + lock (_readLock) + { + return IsOpen || _ringBuffer.AvailableForReading > 0 + ? _ringBuffer.AvailableForReading + : 0; + } + } + + public override byte[] PeekReadableBytes(int numBytes) + { + if (numBytes <= 0) + { + return EmptyBytes; + } + lock (_readLock) + { + if (_ringBuffer.AvailableForReading < numBytes) + { + throw new TransportException( + $"Requested {numBytes} bytes but only {_ringBuffer.AvailableForReading} available"); + } + return _ringBuffer.Peek(numBytes); + } + } + + public override byte[] Read(int numBytes) + { + if (numBytes <= 0) + { + return EmptyBytes; + } + lock (_readLock) + { + if (_ringBuffer.AvailableForReading < numBytes) + { + throw new TransportException( + $"Requested {numBytes} bytes but only {_ringBuffer.AvailableForReading} available"); + } + return _ringBuffer.Read(numBytes); + } + } + + public override void Write(byte[] bytes) + { + if (bytes == null || bytes.Length == 0) + { + return; + } + + lock (_writeLock) + { + EnsureOpen(); + try + { + var offset = 0; + while (offset < bytes.Length) + { + offset += _socket.Send(bytes, offset, bytes.Length - offset, SocketFlags.None); + } + } + catch (ObjectDisposedException) when (Volatile.Read(ref _open) == 0) + { + // A concurrent Close() disposed the socket mid-write: normal shutdown. + } + catch (SocketException e) + { + throw new TransportException("Failed to write data", e); + } + } + } + + public override void Close() + { + // Run the shutdown exactly once even under concurrent/repeated calls. + if (Interlocked.CompareExchange(ref _open, 0, 1) != 1) + { + return; + } + + // Deliberately takes no locks: closing the socket is what unblocks the read + // loop. Taking _writeLock first would deadlock against a blocked writer. + _shutdown.Cancel(); + try + { + _socket.Shutdown(SocketShutdown.Both); + } + catch (SocketException) + { + // Already torn down by the peer; nothing to do. + } + catch (ObjectDisposedException) + { + } + finally + { + _socket.Dispose(); + // Do not wait on the read loop when Close() is called from inside it + // (e.g. a disconnect listener closing the connection). + if (_readLoop != null && !_readLoop.IsCompleted && Task.CurrentId != _readLoop.Id) + { + _readLoop.Wait(TimeSpan.FromSeconds(1)); + } Review Comment: Close() attempts to avoid waiting on the read loop when called from inside it by comparing Task.CurrentId to _readLoop.Id. In async/await code, Task.CurrentId is often null for continuations, so this check can fail and Close() may synchronously wait on its own read-loop task (potential deadlock / 1s stall) when invoked from a data/disconnect listener. ########## plc4net/api/api/ConnectionString.cs: ########## @@ -0,0 +1,184 @@ +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +using System; +using System.Collections.Generic; +using System.Text.RegularExpressions; +using org.apache.plc4net.exceptions; + +namespace org.apache.plc4net.api +{ + /// <summary> + /// A parsed PLC4X connection string: + /// <c>{protocol-code}(:{transport-code})?://{transport-config}(?{parameter-string})?</c> + /// </summary> + /// <remarks> + /// The grammar is kept identical to the Java SPI3 <c>DriverBase.URI_PATTERN</c> so the + /// same connection string addresses the same device from either language. Examples: + /// <c>s7://192.168.0.1</c>, <c>s7:cotp://10.0.0.5:102?remote-rack=0&remote-slot=1</c>, + /// <c>modbus-tcp://10.0.0.9:502?unit-identifier=1</c>. + /// </remarks> + public sealed class ConnectionString + { + private static readonly Regex UriPattern = new Regex( + @"^(?<protocolCode>[a-z0-9\-]*)(:(?<transportCode>[a-z0-9\-]*))?://(?<transportConfig>[^?]*)(\?(?<paramString>.*))?$", + RegexOptions.Compiled); + + // Masks the value of any query parameter whose name looks like it carries a + // credential, so connection strings can be logged safely. + private static readonly Regex SecretParamPattern = new Regex( + @"([?&][^=&]*(?:password|passwd|secret|token)[^=&]*=)[^&]*", + RegexOptions.Compiled | RegexOptions.IgnoreCase); + + private ConnectionString( + string protocolCode, + string transportCode, + string transportConfig, + string paramString, + IReadOnlyDictionary<string, string> parameters) + { + ProtocolCode = protocolCode; + TransportCode = transportCode; + TransportConfig = transportConfig; + ParamString = paramString; + Parameters = parameters; + } + + /// <summary>e.g. "s7", "modbus-tcp".</summary> + public string ProtocolCode { get; } + + /// <summary>e.g. "tcp", "cotp". Null when the string relies on the driver's default.</summary> + public string TransportCode { get; } + + /// <summary>The address the transport consumes, e.g. "192.168.0.1:102".</summary> + public string TransportConfig { get; } + + /// <summary>The raw query string, without the leading '?'. Empty when absent.</summary> + public string ParamString { get; } + + /// <summary>Query parameters, parsed. Keys are case-insensitive.</summary> + public IReadOnlyDictionary<string, string> Parameters { get; } + + public static ConnectionString Parse(string connectionString) + { + if (string.IsNullOrWhiteSpace(connectionString)) + { + throw new PlcConnectionException("Connection string must not be empty."); + } + + var match = UriPattern.Match(connectionString); + if (!match.Success) + { + throw new PlcConnectionException( + "Connection string doesn't match the format " + + "'{protocol-code}(:{transport-code})?://{transport-config}(?{parameter-string})?'"); + } + + var protocolCode = match.Groups["protocolCode"].Value; + if (string.IsNullOrEmpty(protocolCode)) + { + throw new PlcConnectionException("Connection string is missing the protocol code."); + } + + var transportGroup = match.Groups["transportCode"]; + var transportCode = transportGroup.Success && transportGroup.Value.Length > 0 + ? transportGroup.Value + : null; + + var paramString = match.Groups["paramString"].Success + ? match.Groups["paramString"].Value + : string.Empty; + + return new ConnectionString( + protocolCode, + transportCode, + match.Groups["transportConfig"].Value, + paramString, + ParseParameters(paramString)); + } + + private static IReadOnlyDictionary<string, string> ParseParameters(string paramString) + { + var result = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); + if (string.IsNullOrEmpty(paramString)) + { + return result; + } + + foreach (var pair in paramString.Split('&')) + { + if (pair.Length == 0) + { + continue; + } + var separator = pair.IndexOf('='); + if (separator < 0) + { + // A bare flag, e.g. "?verbose" — treat as present-and-true. + result[Uri.UnescapeDataString(pair)] = "true"; + continue; + } + var key = Uri.UnescapeDataString(pair.Substring(0, separator)); + var value = Uri.UnescapeDataString(pair.Substring(separator + 1)); + result[key] = value; + } + return result; + } + + /// <summary> + /// Looks up a parameter, returning <paramref name="defaultValue"/> when absent. + /// </summary> + public string GetParameter(string name, string defaultValue = null) + { + return Parameters.TryGetValue(name, out var value) ? value : defaultValue; + } + + public int GetIntParameter(string name, int defaultValue) + { + var raw = GetParameter(name); + return int.TryParse(raw, out var value) ? value : defaultValue; + } Review Comment: GetIntParameter uses int.TryParse(string) which is culture-sensitive. Connection-string numeric parsing should be culture-invariant (Java uses Integer.parseInt on the decoded string), otherwise locales that use non-ASCII digits or different separators can cause unexpected fallback to defaultValue. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
