[BUG?] Coroutine::cancel not cancelling coroutine.

[ValiDrv]

Please answer these questions before submitting your issue. Thanks!

1. What did you do? If possible, provide a simple script for reproducing the error.

While testing Coroutine::cancel I noticed that the call will only cancel one IO call within the cancelled coroutine.
So either the documentation lost something in translation, or it doesn't actually cancel the coroutine.

<?php
use Swoole\Coroutine;

foreach (['php --ri swoole', 'uname -a', 'php -v', 'gcc -v'] as $cmd) {
    echo "\n----- $cmd ----\n";
    echo `$cmd`;
    echo "\n------------\n";
}

Co\run(function () {
    echo "\n Hello - " . time();
    $c = Co\go(function () {
        Co\defer(function () {
            echo "\n Cancel - " . time();
        });
        Co::sleep(2);
        echo "\n Hello A - " . time() . " " . (Coroutine::isCanceled() ? 'Y' : 'N');
        Co::sleep(2);
        echo "\n Hello B - " . time() . " " . (Coroutine::isCanceled() ? 'Y' : 'N');
    });
    echo "\n Started: $c";
    if (Coroutine::cancel($c) === true) {
        echo "\n Cancelled: $c";
    } else {
        echo "\nswoole_last_error: " . swoole_last_error();
    }
});

2. What did you expect to see?

Expected the coroutine #2 to be cancelled, and not be processed any more

 Hello - 1626696957
 Started: 2
 Cancelled: 2
 Cancel - 1626696957 <-- should be the start time (+0-1sec on second change)

3. What did you see instead?

The first sleep was interrupted, the coroutine continued to run.

 Hello - 1626696957
 Started: 2
 Hello A - 1626696957 Y
 Cancelled: 2
 Hello B - 1626696959 N  <--- at the very least, this should probably be Y for cid #2
 Cancel - 1626696959 <-- it's actually the start time +2sec, since the second `sleep` was not cancelled.

4. What version of Swoole are you using (show your php --ri swoole)?

----- php --ri swoole ----

swoole

Swoole => enabled
Author => Swoole Team <team@swoole.com>
Version => 4.7.0
Built => Jul 16 2021 10:49:14
coroutine => enabled with boost asm context
epoll => enabled
eventfd => enabled
signalfd => enabled
cpu_affinity => enabled
spinlock => enabled
rwlock => enabled
sockets => enabled
openssl => OpenSSL 1.1.1d  10 Sep 2019
dtls => enabled
http2 => enabled
pcre => enabled
zlib => 1.2.11
mutex_timedlock => enabled
pthread_barrier => enabled
futex => enabled
mysqlnd => enabled
async_redis => enabled

Directive => Local Value => Master Value
swoole.enable_coroutine => On => On
swoole.enable_library => On => On
swoole.enable_preemptive_scheduler => Off => Off
swoole.display_errors => On => On
swoole.use_shortname => On => On
swoole.unixsock_buffer_size => 8388608 => 8388608

5. What is your machine environment used (show your uname -a & php -v & gcc -v) ?

----- uname -a ----
Linux ca60602a12a0 4.15.0-46-generic #49-Ubuntu SMP Wed Feb 6 09:33:07 UTC 2019 x86_64 GNU/Linux


----- php -v ----
PHP 8.0.0 (cli) (built: Dec  1 2020 03:14:26) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.0, Copyright (c), by Zend Technologies


----- gcc -v ----
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 8.3.0-6' --with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --program-suffix=-8 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 8.3.0 (Debian 8.3.0-6) 

[NathanFreeman]

Hi. The canceled coroutine would continue to run and coroutine api that cause context switch would return false when canceling a coroutine successfully.

it actually cancel the first coroutine api (Co::sleep(2)).
Coroutine::cancel() cancel Co::sleep() api and resume the target coroutine to continue to run.

    $c = Co\go(function () {
        Co\defer(function () {
            echo "\n Cancel - " . time();
        });
        var_dump(Co::sleep(2)).PHP_EOL;
        echo "\n Hello A - " . time() . " " . (Coroutine::isCanceled() ? 'Y' : 'N');
        var_dump(Co::sleep(2)).PHP_EOL;
        echo "\n Hello B - " . time() . " " . (Coroutine::isCanceled() ? 'Y' : 'N');
    });

Hello - 1626705129
Started: 2
bool(false)

Hello A - 1626705129 Y
Cancelled: 2
bool(true)

Hello B - 1626705131 N

[ValiDrv]

Hi, if what @NathanFreeman answered is the correct answer, then I'm confused.

Coroutine::cancel($c) should a) cancel a coroutine? or b) should cancel whatever IO block that coroutine is currently being suspended for, and resume said coroutine?

If b) is the correct answer, then the ::cancel method name is very misleading (in English, and most Latin based languages), since it actually means it should cancel and forget the coroutine with the $c id (and probably all it's shutdown function).

If this is the case, I suggest the method is renamed to ::cancelIo(...), '::cancelBlock(...)' or something along those lines.
(That L and I in cancelIo camel case doesn't look to good tho...)

[matyhtf]

The Co::cancel() method is to cancel an operation, not killing the coroutine. After cancellation, the coroutine will resume from the suspended state and continue to execute.

[matyhtf]

Thanks for your advice.Let's think about the naming here

[ValiDrv]

Thanks, with that explanation or makes perfect sense.

Except for the misleading name... ( I suggest it's deprecated/renamed before to many people start to use it)