Provide IntoInnerError::into_parts #79673

ijackson · 2020-12-03T20:28:53Z

Hi. This is an updated version of the IntoInnerError bits of my previous portmanteau MR #78689. Thanks to @jyn514 and @m-ou-se for helpful comments there.

I have made this insta-stable since it seems like it will probably be uncontroversial, but that is definitely something that someone from the libs API team should be aware of and explicitly consider.

I included a tangentially-related commit providing documentation of the buffer full behaviiour of &mut [u8] as Write; the behaviour I am documenting is relied on by the doctest for into_parts.

rust-highfive · 2020-12-03T20:28:56Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

m-ou-se

I have made this insta-stable since it seems like it will probably be uncontroversial

I agree that having this functionality is definitely uncontroversial, but the exact name and api (return type, order of the elements) could use an unstable period to get some experience and feedback.

Can you open a tracking issue and change this to #[unstable(.., issue = "..")]? Thanks!

(Edit: Might be good to pick a slightly shorter feature name. ^^ Maybe io_into_inner_error_parts?)

m-ou-se · 2020-12-04T11:35:52Z

library/std/src/io/buffered/mod.rs

+    /// let (err, _writer) = into_inner_err.into_parts();
+    /// assert_eq!(err.kind(), ErrorKind::WriteZero);


Can you think of an example where both parts would be used afterwards?

(This example shows there might be a good reason to (also) add a .into_error().)

I would be very happy to add .into_error() too.

For a hypothetical situation where one might want the underlying writer too: perhaps the underlying writer is some kind of network connection which can be reset to a known state and then reused. Eg, maybe the send operation on a TCP socket has timed out and you can recover by advancing the TCP Urgent Data Pointer in the expectation that this will both get the receiver to start discarding data, and then to resynchronise.

Or maybe the underlying writer has interesting state which you want to use for error reporting. Or resources you wish to reuse. Consider a writer which encapsulates an ssh Channel within an owned ssh connection - maybe the write failed because the peer closed the Channel, but you can do something sensible if you can get the ssh connection out of the Channel which involves peeling off the layers.

Oh I'm sure it'll be useful to have both. I was just wondering if we can come up with a minimal example for in te documentation that shows usage of both the writer and the error.

library/std/src/io/impls.rs

ijackson · 2020-12-04T17:38:36Z

Thanks for the review @m-ou-se.

I agree that having this functionality is definitely uncontroversial, but the exact name and api (return type, order of the elements) could use an unstable period to get some experience and feedback.

That makes sense.

Can you open a tracking issue and change this to #[unstable(.., issue = "..")]? Thanks! Might be good to pick a slightly shorter feature name. ^^ Maybe io_into_inner_error_parts?

Done all that. Thanks for the suggestions. I have done these as git autosquash commits on top of my branch, in the expectation that you'll want me to squash it down later.

I also added into_error (which is actually what prompted me to do this MR at all). And I extended the doctest example for into_parts to show use of the recovered writer.

While doing this I noticed that there is no way to get a failing innner writer out of a BufWriter. You can call into_inner but it will just keep failing and there is no way to clear or retrieve the data. I will do another MR for BufWriter::into_raw_parts.

m-ou-se

Nice, thanks!

While doing this I noticed that there is no way to get a failing innner writer out of a BufWriter. You can call into_inner but it will just keep failing and there is no way to clear or retrieve the data. I will do another MR for BufWriter::into_raw_parts.

Ah, interesting. Looks like this part of the library hasn't gotten the attention it needs. Thanks for doing this. :)

I wonder if it makes sense to (also?) add a way to get the underlying Writer on IntoInnerError. Or maybe .into_parts() should give the buffer and the Writer, instead of the BufWriter? I'll leave that up to you to think about. ^^

I have a few more tiny nits below, after which we can merge this. Feel free to directly squash the commits.

library/std/src/io/buffered/mod.rs

library/std/src/io/impls.rs

library/std/src/io/buffered/mod.rs

ijackson · 2020-12-04T18:31:49Z

While doing this I noticed that there is no way to get a failing innner writer out of a BufWriter. You can call into_inner but it will just keep failing and there is no way to clear or retrieve the data. I will do another MR for BufWriter::into_raw_parts.

Ah, interesting. Looks like this part of the library hasn't gotten the attention it needs. Thanks for doing this. :)

Thanks for the encouragement. That is #79705 now.

I wonder if it makes sense to (also?) add a way to get the underlying Writer on IntoInnerError. Or maybe .into_parts() should give the buffer and the Writer, instead of the BufWriter? I'll leave that up to you to think about. ^^

IntoInnerError is parameterised over the outer writer, not the inner one, so it doesn't know it contains a BufWriter. In theory it might be anything. So I think the unpeeling has to first get the BufWriter out of the IntoInnerError, and then unpick the BufWriter with my new into_raw_parts.

I have a few more tiny nits below, after which we can merge this. Feel free to directly squash the commits.

Wilco, thanks.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

In particular, IntoIneerError only currently provides .error() which returns a reference, not an owned value. This is not helpful and means that a caller of BufWriter::into_inner cannot acquire an owned io::Error which seems quite wrong. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson · 2020-12-04T18:56:28Z

I think I've finished messing about with this now, having fixed those nits and squashed it all down.

m-ou-se · 2020-12-04T19:01:11Z

Thanks!

@bors r+

bors · 2020-12-04T19:01:12Z

📌 Commit b777552 has been approved by m-ou-se

bors · 2020-12-04T22:30:25Z

⌛ Testing commit b777552 with merge a5fbaed...

bors · 2020-12-05T00:42:36Z

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing a5fbaed to master...

rust-highfive assigned m-ou-se Dec 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2020

jyn514 added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 3, 2020

m-ou-se reviewed Dec 4, 2020

View reviewed changes

ijackson mentioned this pull request Dec 4, 2020

Tracking Issue for IntoInnerError::into_parts etc. (io_into_inner_error_parts) #79704

Closed

3 tasks

m-ou-se removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Dec 4, 2020

m-ou-se reviewed Dec 4, 2020

View reviewed changes

library/std/src/io/buffered/mod.rs Outdated Show resolved Hide resolved

library/std/src/io/impls.rs Outdated Show resolved Hide resolved

library/std/src/io/buffered/mod.rs Outdated Show resolved Hide resolved

std: impl of Write for &mut [u8]: document the buffer full error

db5d697

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson force-pushed the intoinnerintoinnererror branch from c7800a7 to 9a2b745 Compare December 4, 2020 18:39

ijackson added 2 commits December 4, 2020 18:43

IntoInnerError: Provide into_error

b777552

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson force-pushed the intoinnerintoinnererror branch from 9a2b745 to b777552 Compare December 4, 2020 18:43

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 4, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 5, 2020

bors merged commit a5fbaed into rust-lang:master Dec 5, 2020

rustbot added this to the 1.50.0 milestone Dec 5, 2020

ijackson deleted the intoinnerintoinnererror branch December 5, 2020 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide IntoInnerError::into_parts #79673

Provide IntoInnerError::into_parts #79673

ijackson commented Dec 3, 2020

rust-highfive commented Dec 3, 2020

m-ou-se left a comment •

edited

Loading

m-ou-se Dec 4, 2020

ijackson Dec 4, 2020

m-ou-se Dec 4, 2020

ijackson commented Dec 4, 2020

m-ou-se left a comment

ijackson commented Dec 4, 2020

ijackson commented Dec 4, 2020

m-ou-se commented Dec 4, 2020

bors commented Dec 4, 2020

bors commented Dec 4, 2020

bors commented Dec 5, 2020

		/// let (err, _writer) = into_inner_err.into_parts();
		/// assert_eq!(err.kind(), ErrorKind::WriteZero);

Provide IntoInnerError::into_parts #79673

Provide IntoInnerError::into_parts #79673

Conversation

ijackson commented Dec 3, 2020

rust-highfive commented Dec 3, 2020

m-ou-se left a comment • edited Loading

Choose a reason for hiding this comment

m-ou-se Dec 4, 2020

Choose a reason for hiding this comment

ijackson Dec 4, 2020

Choose a reason for hiding this comment

m-ou-se Dec 4, 2020

Choose a reason for hiding this comment

ijackson commented Dec 4, 2020

m-ou-se left a comment

Choose a reason for hiding this comment

ijackson commented Dec 4, 2020

ijackson commented Dec 4, 2020

m-ou-se commented Dec 4, 2020

bors commented Dec 4, 2020

bors commented Dec 4, 2020

bors commented Dec 5, 2020

m-ou-se left a comment •

edited

Loading