Fixes issue with non-seekable sources breaking StreamEncryptor and StreamDecryptor handling #14
Conversation
…ich cannot seek or tell
…ernal.utils.streams
…cryptionStream/DecryptionStream handle non-seekable input streams properly
… tell() calls on source streams when handling framed messages
| self._source_stream = source_stream | ||
| self._duplicate_api() | ||
|
|
||
| def _duplicate_api(self): |
There was a problem hiding this comment.
Might just be that Python isn't my daily driver, but I want to ask about a few things:
- Intentionally dropping private-hinted methods: If the language idiom is that privacy is a hint, not a requirement, isn't there a risk that this will break callers of wrapped streams in funky ways? (I know this is also marked as internal, but if it's hints all the way down...)
- Naming is a bit squishy: you refer to
attributeson the LHS andmethods on the RHS, but it looks like you're getting everything not prefixed with_, not only methods - Again this might be at least partially an idiom thing, but rather than trying to assume the methods and attributes of the
source_stream, would it make sense to havePassThroughStreamitself inherit from the base stream type (if such a thing practically exists) and have each of the defined stream methods delegate to the underlyingsource_stream? Or, if not, should it instead copy everything?
For example: say we get a stream object CoolStream that is itself customized and has a (for example) _report_bytes_() API that is called by the local implementation of write(). PassThroughStream copies write() but not _report_bytes(), so when write() is invoked, it unexpectedly fails. Alternatively, if PassThroughStream is say an IOBase (for example; might not be the right choice) that has all the standard stream methods, PassThroughStream.write() can call source_stream.write() and treat the wrapped stream as more of a black box. I think that would drop some edge cases but preserve intent/functionality here?
There was a problem hiding this comment.
Good catch on the attributes/methods. Fixed that.
The problem with just subclassing io.BaseIO is that these are really meant to front file-like objects, not just streams (fixed that terminology in the comments too). File-like objects (of which streams are a subset) have a consistent API but it is also quite large, so I would rather avoid manually duplicating it.
Effectively what this is doing is duplicating the public API of _source_stream onto the instance of PassThroughStream in the form of pointers to the attributes on _source_stream. This makes it so that any public interactions with the instance of PassThroughStream behave exactly as they would if they were made to _source_stream directly. Behavior of calls inside _source_stream are unaffected, as they are still happening within the context of that (unmodified) object.
Aside from the obvious overrides, the only place where this should introduce any unexpected behavior is if someone is making calls to private-hinted attributes on _source_stream, and I think that is a safe place to draw the line, and it matches with the convention laid out in PEP8.
There was a problem hiding this comment.
Per offline discussion: replacing PassThroughStream with ProxyObject from wrapt.
There was a problem hiding this comment.
Yeah, sorry for my confusion on the API duplication, I did not remember that interaction correctly. wrapt is a good find, that turned out as a nice clean drop-in.
… fix references to "streams" to correctly state "file-like" objects
…re dunder conflict
|
Minor update to change internal variable for As a side note, the Travis builds are all failing because #16 isn't present in this PR. |
As detailed in #13 : non-seekable sources were found to break
StreamEncryptorandStreamDecryptorhandling. This PR contains changes required to address this issue as well as removing any need for source stream to be seekable when handling framed messages.Test results: https://gist.github.com/mattsb42-aws/a1714da88d2ab83bf3e1ed6a1cec4b59 (Travis setup coming soon).