CancellationToken in InitializeAsync, ReadAsync and WriteAsync don't cancel the task after the time-out

[fmartens]

First of all, thanks for the great work on libplctag and libplctag.NET.

#72 updated InitializeAsync, ReadAsync and WriteAsync but it did not update the 2 occurrences of token in the inner using:

using (token.Register(() => Abort()))
{
  while (GetStatus() == Status.Pending)
  {
    await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, token);
  }
}

token should be replaced with cts.Token:

using (cts.Token.Register(() => Abort()))
{
  while (GetStatus() == Status.Pending)
  {
    await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, cts.Token);
  }
}

to have the task canceled after the configured time out.

We know that Task.Delay() throws an TaskCanceledException when the time-out expires. Wouldn't it be better to catch the TaskCanceledException and throw a LibPlcTagException(Status.ErrorTimeout) instead?

using (cts.Token.Register(() => Abort()))
{
	while (GetStatus() == Status.Pending)
	{
		try
		{
			await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, cts.Token);
		}
		catch (TaskCanceledException)
		{
			throw new LibPlcTagException(Status.ErrorTimeout);
		}
	}
}

PS: #96 might also be related to this issue, I experienced similar behavior while I was testing.

[timyhac]

Hi @fmartens - nice work!

I've implemented the first part of your message which is a bug, thanks for going through the history and understanding what happened - really appreciate that!.

I'm not 100% convinced on the second bit, the Statuses correspond to status codes from libplctag core - the Status.ErrorTimeout has a specific meaning in terms of what libplctag core thinks that means, which might be different / diverge in the future from the async-timeout meaning.

Lets keep the conversation going because I'm not 100% against it either.

[kyle-github]

If you hand the core control and tell it how long you are willing to wait and that time is exceeded, then you'll get that error status. This may get a little more nuanced as I rejigger the internals into a more event-driven logic flow, but that is really all PLCTAG_ERR_TIMEOUT means.

int rc = plc_tag_read(tag_handle_id, 3000);
if(rc == PLCTAG_ERR_TIMEOUT) {
  fprintf(stderr, "Barf! No response from the PLC after 3 seconds.   Did you plug in the Ethernet cable?\n");
  exit(PLCTAG_ERR_TIMEOUT);
}

If you use your own async approach on top of the core's async calls, you can redefine what timeout means all you want. It has been a while since I looked through the .Net code but if you are not using the core's timeout mechanism, then by all means go ahead and make this more sane and idiomatic for C# users!

Basically if you only pass zero as the timeout argument to those functions that take one, you are purely running async and I am not going to return that error to you. In those circumstances the application/wrapper is saying that it will determine when a timeout occurs and the core library will just keep trying until you tell it to stop.

[fmartens]

@kyle-github and @timyhac , thank you both for the swift responses.

@kyle-github , thank you for your explanation. I went through the libplctag code and it gave me the impression that it would be perfectly fine to throw a LibPlcTagException(Status.ErrorTimeout).

@timyhac: #116 misses await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, cts.Token);, it still uses await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, token);

@timyhac, is there any plan to update the nuget soon?

[timyhac]

Thanks for the pickup!

The only issue is that we'll need to separate the exception by its source,

1. Timeout (after sleeping on it, I'm less against throwing a LibPlcTagException than yesterday - we do have a separation layer between the core statuses, and the wrapper statuses, so if we need to they can diverge in the future.)
2. User-supplied CancellationToken was cancelled (it wouldn't make sense to throw a LibPlcTag exception in this case).

That use of Task.Delay is actually just a hack because we haven't implemented proper event-driven logic with the Read/Write/Init - it just waits 2ms. The timeout for these operations is driven by cts.CancelAfter(Timeout).

What if we do something like this:

public async Task InitializeAsync(CancellationToken token = default)
{
            
    ThrowIfAlreadyDisposed();
    ThrowIfAlreadyInitialized();

    using (var cts = CancellationTokenSource.CreateLinkedTokenSource(token))
    {
        cts.CancelAfter(Timeout);

        var attributeString = GetAttributeString();

        var result = plctag.plc_tag_create(attributeString, 0);
        if (result < 0)
            throw new LibPlcTagException((Status)result);
        else
            tagHandle = result;

        using (cts.Token.Register(() => {
            Abort();
            token.ThrowIfCancellationRequested();                                    // User supplied token
            throw new LibPlcTagException(Status.ErrorTimeout);              // Else it must have been a timeout
        }))
        {
            while (GetStatus() == Status.Pending)
            {
                await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, cts.Token);
            }
        }

        ThrowIfStatusNotOk();

        IsInitialized = true;
    }
}

[kyle-github]

You could use the callback API to have the core library tell you when status changes. I am happy to extend/add to it if there is something missing in it! As I remake the core logic around event-driven flows, it is clear that there is a lot of room for small improvements that will add up.

[fmartens]

@timyhac , i've tested your suggestion and it results in an unhandled exception.Wouldn't it be better to do:

using (cts.Token.Register(() =>Abort()))
{
	while (GetStatus() == Status.Pending)
	{
		try
		{
			await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, cts.Token);
		}
		catch (TaskCanceledException)
		{
			if (token.IsCancellationRequested)
			{
				throw;   // User supplied token
			}

			throw new LibPlcTagException(Status.ErrorTimeout); // Else it must have been a timeout
		}
	}
}

I've read https://bengribaudo.com/blog/2018/02/08/4360/understanding-cancellation-callbacks and an unhandled exception handler is not what you are looking for I quess.

[timyhac]

Yep okay, it reads better as well.

Where does the exception propagate from btw?

[fmartens]

Sorry about the delay.

Not quite sure if I understand your question but it's the LibPlcTagException which is thrown from the timer thread that executes:

  Abort();
  token.ThrowIfCancellationRequested();
  throw new LibPlcTagException(Status.ErrorTimeout);

[timyhac]

Hi @fmartens - I've started work on this and pushed this branch
.

It involves a reasonable refactor of this class, and one thing I would like to do before merging this change is to implement unit tests, particularly around the garbage collection, async behaviour which are the primary things that the .NET wrapper extends above the core library (async with timeout does not exist in core library).
This work has started: #121

[timyhac]

Hi @fmartens - I'd appreciate if you could help develop some unit tests to check for correct behaviour. The work has started here:
https://github.com/libplctag/libplctag.NET/blob/%23115/src/libplctag.Tests/AsyncTests.cs

[fmartens]

@timyhac, I'd be more than happy to contribute to this but I need to find some time to prepare and get familiar with Moq.

[timyhac]

Even helping develop the scenarios that need to be tested would be a big help.
I'm thinking that, in order to close out #115 we just develop tests that check that specific scenario. As a separate item we can look at increasing test coverage.

I've got 3 tests so far, I'm not confident the case is covered properly but it is better than nothing. With the old code we don't get a timeout, but with the new code we do (at least with the unit tests, I haven't tried any manual testing yet).

I've found it quite difficult to learn moq, the official documentation is a bit lacking. I guess there would be books/courses to buy, but I've been relying on free stuff found on the web so far.

[timyhac]

@fmartens - hi there. The work has been merged into master but its not going to be released until the string API has been tested working, and a second pair of eyes looks at the async work. I'd love if you could take a look/test https://github.com/libplctag/libplctag.NET/blob/master/src/libplctag/NativeTagWrapper.cs#L274

[timyhac]

@fmartens - hi there, there is a beta package available on nuget with fixes: https://www.nuget.org/packages/libplctag/1.0.4-beta.1

[fmartens]

I'm quite busy at the moment and I am still learning Moq but the unit tests are still on my todo list though.