Skip to content

fix(http2): finish() and terminate() must not hang after a failed socket write - #2014

Draft
mosuem wants to merge 1 commit into
mosum/http2-5-frame-reader-cancel-and-goaway-exceptionfrom
mosum/http2-5b-buffered-sink-done-after-write-failure
Draft

mosuem wants to merge 1 commit into
mosum/http2-5-frame-reader-cancel-and-goaway-exceptionfrom
mosum/http2-5b-buffered-sink-done-after-write-failure

Conversation

@mosuem

@mosuem mosuem commented Oct 8, 2026

Copy link
Copy Markdown
Member

Note

This PR was generated by an AI coding agent (Jetski) on behalf of @mosuem.

Summary

BufferedSink (the sink behind FrameWriter) defined its completion as

_doneFuture = Future.wait([_controller.stream.pipe(dataSink), dataSink.done]);

For a dart:io Socket, done only completes after an explicit close(). When a write fails — typically SocketException: Connection reset by peer because the peer already closed the connection — addStream completes with the error, Stream.pipe does not close the sink, and Socket.done never completes. Future.wait (non-eager) then waits forever, and so does everything built on doneFuture: FrameWriter.close(), Connection.finish(), Connection.terminate(), ClientPool.terminate() / Http2Client.closed, and grpc-dart's ClientChannel.shutdown() (which awaits transport.finish()).

pipe itself already resolves through dataSink.close(), i.e. through done, in the success case, so the extra wait only ever mattered in the failure case — where it hangs.

Changes

  • lib/src/async_utils/async_utils.dart: doneFuture completes once the pipe has finished. A failed write completes it normally (like a cancelled sink already did): the connection is dead and the owner learns about it through the incoming side (Connection also terminates itself when doneFuture completes).

Test Verification (Fails Before $\rightarrow$ Passes After)

  • buffered-sink-done-after-failed-write in test/src/async_utils/async_utils_test.dart, using a FailingSink that behaves like a reset socket (addStream fails, done never completes without close()).
  • finish-and-terminate-complete-when-the-socket-write-fails in test/client_test.dart: ClientTransportConnection.viaStreams(..., FailingSink()), then finish() / terminate() must complete.

Before fix:

00:02 +0 -1: test/src/async_utils/async_utils_test.dart: async_utils buffered-sink-done-after-failed-write [E]
  TimeoutException after 0:00:02.000000: Future not completed
00:02 +0 -2: test/client_test.dart: client-tests client-errors finish-and-terminate-complete-when-the-socket-write-fails [E]
  TimeoutException after 0:00:02.000000: Future not completed

After fix:

00:00 +2: All tests passed!

@mosuem
mosuem added this pull request to stack #2016 October 8, 2026 10:42
@mosuem
mosuem force-pushed the mosum/http2-5b-buffered-sink-done-after-write-failure branch from 3a2ff2d to 6a45c6d Compare October 8, 2026 12:04
@mosuem
mosuem force-pushed the mosum/http2-5b-buffered-sink-done-after-write-failure branch from 6a45c6d to 001d14f Compare October 9, 2026 07:54
@mosuem
mosuem removed this pull request from stack #2016 October 9, 2026 07:55
@mosuem
mosuem added this pull request to stack #2020 October 9, 2026 07:55
@mosuem
mosuem force-pushed the mosum/http2-5b-buffered-sink-done-after-write-failure branch from 001d14f to b64e5fd Compare October 9, 2026 08:18
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

PR Health

Coverage ✔️
File Coverage
pkgs/http2/lib/src/async_utils/async_utils.dart 💚 100 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ✔️
// Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/http_multi_server/test/cert.dart

This check can be disabled by tagging the PR with skip-license-check.

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
http2 Non-Breaking 3.1.0 3.2.0-wip 3.2.0-wip ✔️

This check can be disabled by tagging the PR with skip-breaking-check.

Unused Dependencies ✔️
Package Status
http2 ✔️ All dependencies utilized correctly.

For details on how to fix these, see dependency_validator.

This check can be disabled by tagging the PR with skip-unused-dependencies-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

…ket write

> [!NOTE]
> This PR was generated by an AI coding agent (Jetski) on behalf of @mosuem.

### Summary

`BufferedSink` (the sink behind `FrameWriter`) defined its completion as

```dart
_doneFuture = Future.wait([_controller.stream.pipe(dataSink), dataSink.done]);
```

For a `dart:io` `Socket`, `done` only completes after an explicit `close()`. When a write fails — typically `SocketException: Connection reset by peer` because the peer already closed the connection — `addStream` completes with the error, `Stream.pipe` does *not* close the sink, and `Socket.done` never completes. `Future.wait` (non-eager) then waits forever, and so does everything built on `doneFuture`: `FrameWriter.close()`, `Connection.finish()`, `Connection.terminate()`, `ClientPool.terminate()` / `Http2Client.closed`, and grpc-dart's `ClientChannel.shutdown()` (which awaits `transport.finish()`).

`pipe` itself already resolves through `dataSink.close()`, i.e. through `done`, in the success case, so the extra wait only ever mattered in the failure case — where it hangs.

### Changes
- **`lib/src/async_utils/async_utils.dart`**: `doneFuture` completes once the pipe has finished. A failed write completes it normally (like a cancelled sink already did): the connection is dead and the owner learns about it through the incoming side (`Connection` also terminates itself when `doneFuture` completes).

### Test Verification (Fails Before $\rightarrow$ Passes After)

- `buffered-sink-done-after-failed-write` in `test/src/async_utils/async_utils_test.dart`, using a `FailingSink` that behaves like a reset socket (`addStream` fails, `done` never completes without `close()`).
- `finish-and-terminate-complete-when-the-socket-write-fails` in `test/client_test.dart`: `ClientTransportConnection.viaStreams(..., FailingSink())`, then `finish()` / `terminate()` must complete.

**Before fix:**
```text
00:02 +0 -1: test/src/async_utils/async_utils_test.dart: async_utils buffered-sink-done-after-failed-write [E]
  TimeoutException after 0:00:02.000000: Future not completed
00:02 +0 -2: test/client_test.dart: client-tests client-errors finish-and-terminate-complete-when-the-socket-write-fails [E]
  TimeoutException after 0:00:02.000000: Future not completed
```

**After fix:**
```text
00:00 +2: All tests passed!
```
@mosuem
mosuem force-pushed the mosum/http2-5b-buffered-sink-done-after-write-failure branch from b64e5fd to a59c01f Compare October 9, 2026 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant