Migrate host_task object management to a thread pool based approach

[ndgrigorian]

Inspired by comment in: AdaptiveCpp/AdaptiveCpp#1915

we move away from use of host_task, which enables compatibility with AdaptiveCpp, and fixes long-standing undefined behavior of calling sycl::free from within a host_task. The host_task decrementing the refcount would cause the USMDeleter to be called in its scope, which is undefined behavior under the SYCL spec

• Have you provided a meaningful PR description?
• Have you added a test, reproducer or referred to an issue with a reproducer?
• Have you tested your changes locally for CPU and GPU devices?
• Have you made sure that new changes do not introduce compiler warnings?
• Have you checked performance impact of proposed changes?
• Have you added documentation for your changes, if necessary?
• Have you added your changes to the changelog?
• If this PR is a work in progress, are you opening the PR as a draft?

[github-actions]

View rendered docs @ https://intelpython.github.io/dpctl/pulls/2359/index.html

[coveralls]

coverage: 74.501% (+0.04%) from 74.462% — feature/drop-host-task-object-management into master

[antonwolfy]

Missing Py_DECREF(args) leading to reference leak, while it is handled properly in keep_args_alive.

[ndgrigorian]

looks like it was never present...

[antonwolfy]

@vlad-perevezentsev, is that intended in dpnp we are using the local copy of that function dpnp::utils::keep_args_alive instead of dpctl vendored function?

Btw, due to that there is no real impact on dpnp due to that dpctl migration change, except new deprecation wanrnings.

[antonwolfy]

There might be a refcount leak if an exception is thrown before/during submit() in the public template.
Needs a scope guard that dec_refs the collected handles unless submit() succeeded.

[antonwolfy]

That might throw bad_alloc, but outside try/catch block

[antonwolfy]

This is AI agentic comment which seems having fair concern on ODR pitfall with real example.

The setup, now confirmed:

• num_threads is baked into an inline header: static constexpr std::size_t num_threads = DPCTL_KEEP_ALIVE_POOL_SIZE; (keep_alive_pool.hpp:56), default 4 (:41-43).
• The macro is applied PRIVATE to the _sycl_queue target only (dpctl/CMakeLists.txt:208-210), with the comment "only it needs to know the pool size." So _sycl_queue's TU compiles the header with, say, =16; every other TU that includes dpctl4pybind11.hpp → detail/keep_alive_pool.hpp (other dpctl extensions, and downstream dpnp / dpctl.tensor / numba-dpex) sees the header default of 4.
• The KeepAlivePool() constructor (:94-99) is an inline member function (external linkage) that uses num_threads. The ODR requires all TUs to see an identical definition — but its definition differs (loop bound 16 vs 4). Same for the inline local_instance(). This is an ODR violation; the linker silently keeps one arbitrary definition and discards the rest.

Real example where it bites:

1. dpctl built with -DDPCTL_KEEP_ALIVE_POOL_SIZE=16. Only _sycl_queue.so's TU has the =16 definition; the installed header still reads 4.
2. A future dpnp adopts dpctl's pool and, in its own .so, includes the header (sees 4) and calls KeepAlivePool::local_instance() directly (it's public, so nothing stops it) to submit its own release tasks.
3. The program now links two conflicting definitions of the constructor — one spawning 16 threads, one spawning 4. Because the symbols are inline/external and both TUs ODR-use them, the dynamic linker collapses them to one arbitrary winner (depends on link order / symbol interposition). dpctl configured 16 but may get a pool of 4, or the reverse — non-deterministic across platforms and build configs, and formally UB.

Why it's benign today: the constructor / local_instance() are ODR-used only inside _sycl_queue (via _async_dec_ref.hpp, included by exactly one TU). Everyone else reaches the pool solely through get_keep_alive_pool() → KeepAlivePool_Get() (dpctl4pybind11.hpp:309-322) — a C-API function pointer exported by _sycl_queue, which routes to _sycl_queue's own correctly-sized instance. So only one definition is actually emitted and the violation stays latent. It goes live the moment any other TU ODR-uses the constructor (a direct local_instance() call).

How to make it robust: don't bake a macro-varying value into an inline definition. Options: read the size once at runtime (env var) inside a single non-inline function defined only in _sycl_queue; or make local_instance() private / non-public so downstream cannot instantiate the pool and is forced through KeepAlivePool_Get(); or at minimum document that the header default is authoritative for any TU other than _sycl_queue, so a customized size only takes effect for the canonical instance.

[antonwolfy]

It might be helpful to add tests exercise the actual pool release path (refcount returns to baseline after gating events complete), the shutdown/finalization guard:

import sys, gc, time, weakref, dpctl

def test_keep_args_alive_releases_after_event(q):
    class S: pass
    obj = S()
    ref = weakref.ref(obj)
    base = sys.getrefcount(obj)
    e = q.submit_barrier()            # a real, completable SyclEvent
    dpctl.keep_args_alive((obj,), [e])
    assert sys.getrefcount(obj) == base + 1   # incref happened synchronously
    e.wait()
    del obj
    # with a drain hook: dpctl.utils._wait_pool_idle(); assert ref() is None
    for _ in range(500):              # fallback: bounded poll
        gc.collect()
        if ref() is None: break
        time.sleep(0.01)
    assert ref() is None              # pool ran the deferred decref

def test_shutdown_with_pending_keepalive():
    script = r"""
import dpctl, faulthandler; faulthandler.enable()
q = dpctl.SyclQueue()
e = q.submit_barrier()          # or a long kernel to leave work in-flight
class S: pass
dpctl.keep_args_alive((S(),), [e])
# intentionally do NOT wait -> exercise release racing interpreter teardown
"""
    p = subprocess.run([sys.executable, "-c", script],
                       timeout=60, capture_output=True)
    assert p.returncode == 0
    assert b"Fatal Python error" not in p.stderr and b"Aborted" not in p.stderr

[ndgrigorian]

My concern with a test like this is that it's reliant on specific work happening on the queue, with specific timing. Tests like this feel very unreliable and flaky.

It may be more possible now that we need a dummy event again in some cases.