Usage of `FnOnce` in `SinkMapExt::sink_map_err`

[twittner]

SinkMapExt::sink_map_err accepts an FnOnce to map the error type at most once. A second error mapping will cause a panic because the mapping function has been consumed. As far as I can tell this dates back to futures-0.1 times when the Sink API documentation was quite explicit about possible panic conditions, e.g. "This method may panic in a few situations [...] - A previous call to start_send or poll_complete yielded an error." [1]

The current API documentation on the other hand is silent about panics, it only states that "In most cases, if the sink encounters an error, the sink will permanently be unable to receive items." [2] However using sink_map_err on any Sink type effectively prevents client code from calling any of its methods after an error has occurred if avoiding the panic from SinkMapErr is the goal.

Now, my question is ― should the Sink API documentation be updated to better express the panic conditions as it used to be the case, or should SinkMapExt::sink_map_err perhaps be changed to accept an FnMut instead?

[alokedesai]

Any update on this? The differences in invariants between SinkMapErr (which assumes a Sink can't be written to after it has errored) and Sink (which leaves this up to each implementation of Sink) is problematic.

Would you be open to accepting a PR to change SinkMapErr to take in an FnMut instead of an FnOnce? Unfortunately, it would be a breaking change, but I think it's worthwhile to ensure there aren't unexpected panics.

[gdesmott]

Agree this is a serious defect in the current API. We hit multiple confusing panics because of it.