Possible deadlock using serial communication

[RevZero]

NuGet Package Version

3.4.0

Payment service provider

irrelevant

Payment terminal

irrelevant

Communication Logs

13:35:17.560 info: Portalum.Zvt.ZvtClient[0] RegistrationAsync - Execute
13:35:17.564 dbug: Portalum.Zvt.ZvtClient[0] SendCommandAsync - Send command to PT
13:35:17.568 dbug: Portalum.Zvt.SerialPortDeviceCommunication[0] SendRaw - 10-02-06-00-10-10-00-00-00-BE-09-78-03-01-06-06-26-04-0A-02-06-D3-10-03-D7-DE

More information

Should be easy to reproduce, by just sending a command without any device attached.

The SerialPortDeviceCommunication.SendAsync() may never return from

Portalum.Zvt/src/Portalum.Zvt/SerialPortDeviceCommunication.cs

Line 183 in 79ae5e0

var byte1 = (byte)this._serialPort.ReadByte();

I am not sure, why ReadByte() doesn't return -1 as stated in the MS documentation.

The byte, cast to an Int32, or -1 if the end of the stream has been read.

To avoid the synchronous blocking, we could check if there is any data to be read before calling ReadByte() like this:

if (_serialPort.BytesToRead <= 0)
{
    await Task.Delay(10, cancellationToken).ConfigureAwait(false);
    continue;
}

However, there is still the issue, that if no timeout based CancellationToken was supplied to the SendAsync(), it will never exit the loop. Looking at the code, there are quite a lot of calls where the token is ommitted:

The call from SendCommandAsync() also does not implement an internal timeout. The user supplied token will be considered, but the timeout should be an implementation detail and not something, the user of the API has to consider by providing a timeout.

I am not sure about the best approach here. The ACK seems to belong to the serial protocol layer, so SendAsync() should probably implement a timeout directly instead of relying on a CancellationToken.

[tinohager]

Hi, we have never used serial communication as we control all terminals via TCP. It is therefore quite possible that there is still some potential for improvement here. The question is whether many more terminals will communicate via serial in the future.

[RevZero]

It's understandable that you've focused on LAN so far.

I think there are still used cases for both, at least when talking about USB as a virtual serial port. It benefits from simplicity, at least when the EC device and the "controller" are not physically separated. No potentially changing IP addresses if they are not permanently assigned via DHCP. No address conflicts with static assignment in external networks. Fewer security concerns.