Implement aborting read/write

[Katze719]

No description provided.

[Copilot]

Pull request overview

This PR implements a mechanism to abort blocking read and write operations on serial ports from other threads. The implementation uses POSIX pipes to signal abort requests and integrates them with the existing poll-based timeout mechanism. The abort functionality is registered when a serial port is opened and cleaned up when it is closed.

Key changes:

• Added abort registry infrastructure to manage abort pipes per file descriptor
• Implemented serialAbortRead and serialAbortWrite functions to trigger aborts
• Modified serialRead and serialWrite to check for abort signals during polling
• Removed blocking tcdrain() call from serialWrite to enable abort functionality

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/detail/abort_registry.hpp	Defines AbortPipes structure and functions for managing abort pipes per FD
src/detail/abort_registry.cpp	Implements abort pipe registration, cleanup, and lookup with mutex protection
src/detail/posix_helpers.hpp	Adds drainNonBlockingFd and waitFdReadyOrAbort helpers for abort support
src/serial_abort_read.cpp	Implements serialAbortRead by writing to the read abort pipe
src/serial_abort_write.cpp	Implements serialAbortWrite by writing to the write abort pipe
src/serial_open.cpp	Registers abort pipes for the serial FD after successful configuration
src/serial_close.cpp	Unregisters and closes abort pipes before closing the serial FD
src/serial_read.cpp	Refactored to use waitFdReadyOrAbort and handle abort signals
src/serial_write.cpp	Modified to use waitFdReadyOrAbort, handle aborts, and removed tcdrain()
tests/test_abort.cpp	Comprehensive unit tests for abort read/write functionality
tests/test_serial_arduino.cpp	Added abort read test and environment variable for skipping init delay
integration_tests/ffi_bindings.ts	Added FFI bindings for serialAbortRead and serialAbortWrite
integration_tests/integration_test.ts	Added assertions for abort function symbols
.github/workflows/cpp-tests.yml	Added SERIAL_TEST_SKIP_INIT_DELAY environment variable
.github/workflows/deno-tests.yml	Added SERIAL_TEST_SKIP_INIT_DELAY environment variable

Comments suppressed due to low confidence (1)

src/detail/abort_registry.cpp:127

• There is a race condition between getAbortPipesForFd and unregisterAbortPipesForFd. The function returns a copy of AbortPipes containing file descriptors, but releases the lock before the caller uses those descriptors. If another thread calls unregisterAbortPipesForFd and closes the pipes after the lock is released but before the caller uses the returned file descriptors, the caller will attempt to read from or write to closed file descriptors. This can lead to undefined behavior, including writing to file descriptors that have been reused for other purposes. Consider using reference counting or another synchronization mechanism to ensure the pipes remain valid while in use.

    {
        return std::nullopt;
    }

    std::lock_guard<std::mutex> lock(g_abort_mutex);
    auto iter = g_abort_by_fd.find(fd);
    if (iter == g_abort_by_fd.end())
    {
        return std::nullopt;
    }
    return iter->second;
}
} // namespace cpp_bindings_linux::detail

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The poll result checking does not handle error conditions (POLLERR, POLLHUP, POLLNVAL) on the file descriptors. When poll() returns with these flags set, it indicates an error condition on the file descriptor. The current implementation only checks for POLLIN/POLLOUT and might return 0 (timeout) even when an error occurred. Consider checking for error flags and returning -1 or handling them appropriately to avoid silent failures.

[Copilot]

The function comment should clarify that this function is specifically designed for draining abort pipes and does not invoke error callbacks. It also silently ignores errors other than EAGAIN/EWOULDBLOCK/EINTR, which is intentional for cleanup operations but should be documented to prevent confusion.

[Copilot]

The poll() system call can be interrupted by signals and return -1 with errno set to EINTR. This should be handled by retrying the poll() call in a loop, similar to how EINTR is handled in other parts of the codebase (e.g., drainNonBlockingFd, serialAbortRead, serialAbortWrite). Without this, an abort operation could fail unexpectedly if a signal arrives during polling.