-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Stream - Interface segregation #4724
Comments
@KrzysztofCwalina can you please have a look. |
This would be an awesome addition 👍 |
(1) Useful stuff here but this needs to be simplified considerably. In particular, signatures such as:
suggest that the API design is too complicated. Generics tend to infect calling code. Generics should not be required to pass around streams. I think a bit of runtime discovery is not at all a problem. (2) Maybe we could get most of the benefits with two simple interfaces (3) In general I think that generalizing streams to non-byte use cases sounds useful but I personally have never needed that. Whenever I had a similar need I used (4) Personally, I have needed runtime discovery to see whether the (5) My idea always was to have a
And maybe extensions on Note, that static capabilities can never fully work in case the presence of a capability is dynamic. For example, the ability to write ( |
@GSPP I may have spread the Api out too much so I included a summary at the bottom (1) Addressed further down so you'd just use public static void DoSomething(ISeekableDuplexStream<byte> stream) (2) doesn't cover RW and ownership issues "Who calls disposable". The destructuring with inheritance is to allow only the expected use to be passed on. Imagine if (3) This is a case of definition at the fringes; increasing glue code needed to get 3rd party compnents to work together. You'll use (4) Here you'd want one of the (5) You can open as Does that help? |
(1) yeah, that works but it's kind of a mess. Your implicit interface proposal would help address that but it does not remove the jungle of interfaces. (3) That is true. Better to have a common standard. (4) That does not work with dynamic capability discovery. (5) This breaks down when I want to call a function that needs to behave differently depending on what runtime capabilities the stream has. I might not want to caller to make that decision. (6) (New) Regarding ownership, what about a wrapping stream that allows you to suppress certain calls such as Close and Flush. I have needed both because libraries I was calling were closing and flushing my streams when they were not supposed to. Flushing for example can cause physical disk seeks when all you wanted was flush intermediate streams into the Windows file cache. I solved that with wrapper streams that suppress disposal etc. Maybe we can solve all ownership issues by declaring by convention that streams are always owned and if you don't like that don't do Above I reported that some ZIP libraries accessed For wrapping to work an interface based solution is not enough. Here, in order to pass information about capabilities you need data because you can't dynamically implement an interface. |
(1) Have to work with how the type system currently works and to get the derivations correct. To the implementer you implement one interface repersenting the maximal interface you support. The user uses one interface; which is the minimal interface they require - the compiler does the rest. (4) & (5) just use Stream here; am not suggesting taking it away (6) Trying to avoid wrapping; though conversely you could for (4) & (5) create an abstract base and add the state properties on which then change for runtime discovery and you methods take the abstract base class type - though you would be violating the compile time contract, so just using Stream would probably be better. |
Closing as Channels looks more appropriate https://github.com/davidfowl/Channels |
Stream - Interface segregation
Problem Statement
Stream is a leaky abstraction with its behavior determined at runtime with
CanRead
,CanSeek
,CanTimeout
,CanWrite
andNotSupportedException
rather than compile time contracts.This also means adapter wrapping classes like
TextWriter
and its subclasses, which don't share Stream's interface, are required to enforce consistency; which leads to higher allocations. However their constructors throw so it is still runtime discovery.The wide interface scope of
Stream
also suffers from ownership confusion due to it being passed as aIDisposable
leading to clarifying methods such asAlso its large Api surface is problematic for testing, dependency injection and scope control - as rarely is it desirable to expose a directly derived type; rather than a secondary wrapper for the derived type, in a public Api due to this large surface area.
These effect cause the Api surfaces to be defined at the fringes rather than in the BCL; reducing easy interoperability between 3rd party components.
Ideally the ownership of behavior would be brought back to the BCL which would allow greater component reuse and communication without requiring custom adapters or glue code for each interaction.
Inefficiencies
Creating an HttpContext that supports keepalive; Request, Response and Duplex streams from a socket currently would requires the creation of 4 NetworkStream objects; 3 per request which maybe be several million extra objects created per second.
This is because disposable control of the socket can't be passed on, and Read shouldn't have write ability, and Write shouldn't have read ability.
This situation could be improved by the constructor taking a disposable R+RW interface; easier to mock for testing; for example they could be a file input and a file output combined in a class that implemented the interface. This could then be exposed it to the properties via up-casting to its parent interfaces. Which only requires a single stream object; as shown below.
This ends up being a 75% reduction in allocations aspnet/KestrelHttpServer#437 for the same behavior.
Background
This stemmed from a conversation on twitter; which coincided with some frustrations I was having with with
Stream
at the time.@davkean tweet
@haacked tweet
@benaadams (ben_a_adams on twitter) tweet
@davkean tweet
Which I addressed in this Gist in a non-breaking way that is compatible with Stream.
However that is not my api proposal as if Stream implemented these interfaces it would be violating the Interface contracts; and would still need runtime checks so wouldn't be a step forward. Also some historical methods can be dropped like
Close
as that is now covered byDispose
as these are new interfaces.Proposed API
Base Interfaces
Example use
This by itself is enough to replicate most of the general stream behavior using Generic Constraints; with the type "streamified" by including
IDisposable
as a constraint.Convenience Extensions
The CopyTo convenience functions introduced at 4.0 and 4.5 can be covered by a set of Extension methods.
Convenience Mixins
However, most day to day use would be a combination of the interfaces; and for ease of use rather than using Generic Constraints; as well as allowing for consistent up-casting between the interfaces; the following pre-defined mixins should be declared:
Convenience Stream Mixins
Streams are essentially the above but with the ability to close them; or the
IDisposable
interface. They should also be able to be cast to the above interfaces so they can be passed to methods without allowing ownership change or disposal. These interfaces would be as follows:Example use
More importantly operate directly on hard types without indirection; for example a FileStream would be
Suggested Integration into BCL
Stream
As stated in the opening problem statement
Stream
doesn't not nessarily obey the interface's contracts, depending on its runtime state; soStream
cannot implement these interfaces directly without inadvertently violating their contracts.However, to get widespread adoption, using
Stream
as a source for these interfaced types needs to be supported. Adding a shim object adds unnecessary overhead, which we are trying to avoid. Luckily (unsure) the interfaces can be implemented via hidden internal interfaces forStream
.The addition of AsType methods with checks, then gives an easy route to use the
Stream
abstarct base class as the source and maintain strong interface contracts.TextReader
TextReader
:IBlockingReadStream<char>
,IReadStreamAsync<char>
TextReader
would need the following extra method defined to supportCancellationToken
parameter.TextWriter
TextWriter
:IBlockingWriteStream<char>
,IWriteStreamAsync<char>
,IBlockingWriteStream<string>
,IWriteStreamAsync<string>
TextWriter
would need the following extra methods defined to supportCancellationToken
parameter and thestring
based interface.Use cases
CopyTo & CopyToAsync & Extensions
As shown above as part of the api additions
CopyTo
andCopyToAsync
can have their main method defined as extension methods, but only need the up-cast typesIBlockingReader<T>
,IBlockingWriter<T>
,IReaderAsync<T>
IWriterAsync<T>
and do not need access to the fullIDisposable
type.This also means its automatically defined on types of the same type
T
so anyReader(Async)
can copy to anyWriter(Async)
whether itsStream
derived,TextReader
/TextReader
derived or any other future or 3rd party type.Audio/Video Playback
Pass ISeekableReadStream to main control
Subcontrols only need up-cast interfaces and do not need access to properties and methods they shouldn't be using.
Multi-file bundling, Continuous log processing
Multi-file bundling or continuous log file processing could be implemented with the following class; adding a bunch of files to the
ReadStream
or appending new files over time.Null IO
All forms of Null IO for a datatype can be represented by a single object.
Example usage
Request & Response Streams
Returning to the Request & Response Streams, shown in the inefficiencies section of the problem statement. That demonstrated an improved pure interface approach.
However, with the additional
Stream
AsType changes the entry point can still be maintained and the same result can be achieved; as shown below.Encoding, Decoding
As shown above
TextReader
andTextWriter
can share theStream
Api, but this can be taken further. These interfaces would allow the stream Api to flow though all stages for efficient stream processing, runtime flow construction, compile time checking and a common Api.Execution Pipeline
tl;dr
Add a compile time contract, that can be scope reduced to Streams in a way that has no breaking changes and gives greater flexibility for the future. Let the streams flow...
Issues...
Implementing an internal interface; exposes the internal interface's public inheritance chain on the object. Way to prevent compile time direct casting?
Interfaces have
Async
at the end rather than start asIAsyncReadStream
fits more with theBeginXXX
EndXXX
type async in case that also wants support.Main Api Summary
Changes
The text was updated successfully, but these errors were encountered: