`NIOAsyncChannel.executeThenClose` can lead to lost writes

[FranzBusch]

When using the NIOAsyncChannel.executeThenClose it is easy to lose writes. The following code is prone to this

try await channel.executeThenClose { inbound, outbound in
    try await outbound.write(...)
}

The problem here is that while write is async it doesn't wait for the write to actually hit the socket. We did this to avoid allocating a promise for every single write. However, this also means that the write might be buffered in the pipeline or the channel itself. Once we return from the closure of executeThenClose we are calling channel.close() which leads to a forceful closure of the channel. This means that any potentially buffered write might get dropped.

Even when using outbound half closure this can happen since outbound.finish is not async and not waiting for the half closure to be written out.

A few options that I thought about so far:

• Allocate a promise for every write. This would trade correctness for performance.
• Introduce a new soft close mode in the channel pipeline that allows all writes to get flushed and then close

[weissi]

This is clearly a bug. You have to wait for the write to complete or else you'll violate Structured Concurrency as well the possibility of losing writes.

There's no way out of this.

---

Note, you could use an empty writeAndFlush() at close and wait for that. That would fix this bug but introduce other bugs (like when the user actually does want to just close) & still violates structured concurrency.

There's no fire & forget in Swift Concurrency and we shouldn't invent one.

[FranzBusch]

I agree with that. I am wondering if we should have two methods write and writeWithoutWaiting so that users can choose which one to pick. The latter would have the potential of dropping writes but if a developer knows he is going to write something afterwards anyways then it would help them avoid promise allocations.

[weissi]

I agree with that. I am wondering if we should have two methods write and writeWithoutWaiting so that users can choose which one to pick. The latter would have the potential of dropping writes but if a developer knows he is going to write something afterwards anyways then it would help them avoid promise allocations.

There's not really room for that. The method can't be synchronous (may need to hop threads to enqueue) and async methods should follow Structured Concurrency. Swift Concurrency chose to not support that, I don't think we should work around it. If fire & forget is something that Swift has appetite for, it should provide a facility for that.

[Lukasa]

Is there any reason this shouldn't use the existing close ratchet functionality we have to default to half-closure? We already know the closure state of the system, and half-closure is supposed to be well-ordered with writes (though it isn't always, that's a bug IMO).

[weissi]

Is there any reason this shouldn't use the existing close ratchet functionality we have to default to half-closure? We already know the closure state of the system, and half-closure is supposed to be well-ordered with writes (though it isn't always, that's a bug IMO).

That should also be the case. But regardless, we need to get the correct error at the correct time. So if you try await ...write(...) then this should get you the syscall error.

This also hits an interesting difference between Netty & NIO: Netty will not send outbound I/O errors through the (inbound) errorCaught. The only way to get outbound errors in Netty is via the write future, NIO behaves differently of course (outbound errors usually also come through errorCaught). Regardless, if a user writes in async land they should get their error and we should have attempted to syscall.

[Lukasa]

So the downside with this proposal is that naive use of the async/await APIs will be very slow: essentially all writes count as a full flush, and cannot progress until the write completes. That's not the way NIO works typically: in the ChannelHandler API you keep writing until the writability state changes, and then delay. Because of NIO's use of promises, these two spellings can be separated, but in async/await they can't.

I'm not confident that the cure isn't worse than the disease here.

[FranzBusch]

Is there any reason this shouldn't use the existing close ratchet functionality we have to default to half-closure? We already know the closure state of the system, and half-closure is supposed to be well-ordered with writes (though it isn't always, that's a bug IMO).

The problem with that is that it only works in half closure cases. If you want to do a write, flush then close it's not going to work. Now arguably that's the same in the NIO world where we often do

context.writeAndFlush(..., promise: nil)
context.close()

The above has the same problem that any intermediate handler could buffer the write and the flush and then we just lose the write because we did a hard close at the end.

I'm not confident that the cure isn't worse than the disease here.

I agree with this though. Making every single write require a promise and a flush is going to make the async interfaces incredibly costly. The only thing that I could come up with so far that addresses both the performance + the guarantee that the sys call to write happened is making it async all the way down i.e. #2648

[Lukasa]

I don't think that's accurate either. If you do write/flush/close(mode: .all) then semantically you've already asked for us to drop all the bytes. I appreciate that's a surprise to many users, but it's the logical implication of the API.

If, however, you do write/flush/close(mode: .output) then it is an error for a handler to reorder that close with the write/flush.

[Lukasa]

While we're here, merely guaranteeing that the write syscall happened is still not enough. If write has completed, and then you call close(mode: .all), the kernel may choose to drop the writes.

If you write, and then want to wait for the bytes to go out, the only supported mode is close(mode: .output). Nothing else works, including #2068.

[FranzBusch]

Right, different proposal then. What if we make writer.finish() async? And we attach a promise to the close(mode: .output). That should give us the guarantee back that the writes made it out.

[Lukasa]

Not really enough either. In the case where the local entity begins the shutdown, merely having shutdown(SHUT_WR) return doesn't provide a sufficient guarantee. We still need to wait for ourselves to read EOF. The shutdown ratchet is fundamentally the right API design: if nothing has been closed at the point where executeAndClose fires, we should close mode output and wait for inbound half closure. If the inbound side is already closed, that will upgrade to immediate closure when close(mode: .output) completes, assuming we attach a promise.

We can also rely on cancellation to override theabove "don't lose data" pattern to upgrade it to "shut up and drop the damn thing", though [robably we don't want to wait forever, some amount of "graceful shutdown timeout" should happen, and will need to be configurable.

Ultimately, the original design of NIOAsyncChannel aimed to make half-closure the default behaviour, and we should continue to aim for it as it's the only thing that prevents this kind of issue.

[weissi]

Not really enough either. In the case where the local entity begins the shutdown, merely having shutdown(SHUT_WR) return doesn't provide a sufficient guarantee.

Sure, I understand. But we would still communicate everything the kernel tells us. Yes, that doesn't mean the other side has received it but as least we didn't eat up a return value we could've provided.

Ultimately, the original design of NIOAsyncChannel aimed to make half-closure the default behaviour, and we should continue to aim for it as it's the only thing that prevents this kind of issue.

Yes, half-closure is great but that doesn't imply full fire and forget mode

[Lukasa]

Yes, half-closure is great but that doesn't imply full fire and forget mode

Right, but that's my argument. Back when NIOAsyncChannel used deinit-based cleanup, it would do half-closure more-or-less by default. executeAndClose discarded that choice, which I'm arguing is wrong.

[FranzBusch]

I think we have two orthogonal but related points here.

1. executeThenClose is currently prone to dropping writes. We should try to ratchet this down by doing half closure
2. outbound.write isn't surfacing the I/O errors instead they come in through inbound.next()

For 1. we have a solution in mind but 2. is difficult without creating a promise for every write. However, even in pure NIO we rarely create promises for writes and instead rely on the error coming through errorCaught. That's why we went with bubbling up the error in inbound.next().

[weissi]

2. is difficult without creating a promise for every write.

I don't think there can be a correct solution without a promise for every write. Why don't we want a promise for every write? Swift Concurrency's model forces us into doing this and one extra allocation isn't killing anybody here. We'll be doing a syscall anyway which is orders of magnitude more expensive.

For multi-write things where the allocation would indeed suck we can find different options.