Skip to content

Fix NioAsyncWriter test on concurrency thread pool with single thread #3135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

orobio
Copy link
Contributor

@orobio orobio commented Mar 7, 2025

Motivation:

The testSuspendingBufferedYield_whenWriterFinished test fails on the Android emulator. See also the discussion in #3044.

Modifications:

The test requires at least two threads in the concurrency thread pool because it blocks one task, which waits for another task to set a condition. This PR adds support for running a task executor based on a NIOThreadPool and uses it for the test. Using a custom task executor guarantees that at least two threads are available for the test.

Additionally, the test has been renamed to testWriterFinish_AndSuspendBufferedYield, which is more in line with the other test names.

Result:

The test will pass regardless of the width of the global concurrency thread pool.

@orobio
Copy link
Contributor Author

orobio commented Mar 7, 2025

@finagolfin : Could you verify this fix on the Android emulator?

@Lukasa : I wanted to get some feedback first before writing tests for withNIOThreadPoolTaskExecutor. I will start creating some tests for that function if the fix works, and if this solution is deemed acceptable.

@finagolfin
Copy link
Contributor

Could you verify this fix on the Android emulator?

I can confirm this pull fixes the failing test on the Android emulator, 😃 once I updated the NIO package manifest to add this dependency. Ignore the failing trunk build: a new snapshot was just tagged which requires a three-hour build on my CI, so I manually canceled that trunk build.

The 6.0/6.1 linux runs passed with the emulator set to a single core, meaning this pull fixed that, whereas the mac runs now show other tests failing but this test passes. I don't know what the other mac test failures are about, but I've been seeing a lot of mac flakiness in the last couple hours, so I'm hoping that's a mac CI issue that will go away. 😉

@orobio orobio force-pushed the fix-NIOAsyncWriter-test-on-concurrency-thread-pool-with-single-thread branch from 3b535ec to 4957b3b Compare March 11, 2025 08:26
@orobio
Copy link
Contributor Author

orobio commented Mar 11, 2025

@finagolfin : That is good news. Thank you for verifying! I've added the dependency in Package.swift.

@Lukasa : Would this solution be acceptable? Using NIOThreadPool seemed like the obvious choice, and I designed it to ensure everything is cleaned up by the end of the test. However, incorrect usage could cause withNIOThreadPoolTaskExecutor to wait indefinitely for tasks to finish. I think guaranteed clean-up is a good thing to have for this, but let me know if you'd prefer a different approach.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general I'd accept this for this use-case, yeah. Let's just tweak a few things.

@orobio orobio force-pushed the fix-NIOAsyncWriter-test-on-concurrency-thread-pool-with-single-thread branch from 4957b3b to 66ec6ba Compare March 23, 2025 17:50
orobio added 4 commits March 23, 2025 18:59
The testSuspendingBufferedYield_whenWriterFinished test causes the test
application to hang in the Android emulator. This fix sets a timeout on
waiting for the ConditionLock, so we don't wait indefinitely.

Additionally, the timeout for waiting for both yields being suspended is
increased to make it less likely that an incorrect state is reached.
Provides withNIOThreadPoolTaskExecutor, which runs a task executor based
on a NIOThreadPool with a specified number of threads.
…ndroid emulator

The testSuspendingBufferedYield_whenWriterFinished test requires at least
two threads in the concurrency thread pool because it blocks one task,
which waits for another task to set a condition. In environments where
the global concurrency thread pool doesn’t have at least two threads
available, the test will fail, as observed on the Android emulator running
with a single virtual core (see discussion in apple#3044).

Using a custom task executor guarantees that at least two threads are
available for the test, regardless of the width of the global concurrency
thread pool.
@orobio orobio force-pushed the fix-NIOAsyncWriter-test-on-concurrency-thread-pool-with-single-thread branch from 66ec6ba to 7af7145 Compare March 23, 2025 17:59
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@glbrntt glbrntt enabled auto-merge (squash) March 27, 2025 14:27
@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Mar 27, 2025
// Delay this yield until the other yield is suspended again.
suspendedAgain.lock(whenValue: true)
suspendedAgain.unlock()
func testWriterFinish_AndSuspendBufferedYield() async throws {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being late to the party but I just realized that this test could be completely re-written to make it work without any thread backed executors and fully reliable. What I wanted to achieve with this test originally was to control the order of the yields and their suspension. I think using a manual task executor would make this test 100 times more maintainable and remove any of the locks.

This is how a manual executor looks like:

import DequeModule
import Synchronization

@available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)
final class ManualTaskExecutor: TaskExecutor {
    private let jobs = Mutex<Deque<UnownedJob>>(.init())
    
    func enqueue(_ job: UnownedJob) {
        self.jobs.withLock { $0.append(job) }
    }
    
    func run() {
        while let job = self.jobs.withLock({ $0.popFirst() }) {
            job.runSynchronously(on: self.asUnownedTaskExecutor())
        }
    }
}

With such an executor we should be able to exactly control what child task runs at what point and control when they suspend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting idea. I can have a look at that.

Note that I recently added this test with PR #3044.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry. I thought this was one of my original tests. Regardless I think we can achieve the ordering that we want here by manually controlling the order of execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FranzBusch : I pushed an update to switch to a ManualTaskExecutor, which works nicely. Is this what you had in mind?

One note: I have no knowledge of the versions in Apple's ecosystem, so I just copied the @available line from your example, assuming that it is correct.

auto-merge was automatically disabled April 11, 2025 11:29

Head branch was pushed to by a user without write access

@orobio orobio force-pushed the fix-NIOAsyncWriter-test-on-concurrency-thread-pool-with-single-thread branch from c58b62e to 8afbc3f Compare April 11, 2025 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants