Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Source and Sink extensions for Apple's NSInputStream and NSOutputStream #174
Add Source and Sink extensions for Apple's NSInputStream and NSOutputStream #174
Changes from all commits
c0f042f
13bf3b1
3386ab3
6b4bb80
8ca65a2
2be70a6
54e247c
dccd22a
2ba8bed
d7b8e1d
62757c5
613e2be
e5f5c27
f5dfc1a
c9d1f44
242fda0
ead4f78
f9c9305
18ff1d0
c4eb1b9
56bb0e4
9d53c8e
4e3ff87
aa5830c
6f076d9
f789518
feb3145
3508104
9e71d4b
f40d472
caba9b4
afab5b7
365c354
a19c463
0021412
211c5f5
ae33893
5d14977
5cfafa2
e9fcaeb
50fe63f
89a4f80
e296c01
7b3ab06
c74845e
d2d040d
630423c
110e56c
6d034e9
ed5902e
07318d0
4897741
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how things work under the hood when inheriting NSInputStream/NSOutputStream and calling a non-default initializer (I understand that no-arg initializer is not available here) as according to Apple's docs, these initializers aimed to instantiate some specific stream's subclass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only way I found to appease the requirement
Unable to call non-designated initializer as super constructor
, which is the error the compiler throws if you use the no-arg constructor. This seems to be a conflict between the way Kotlin and Objective-C object inheritance works, where Objective-C can return a subclass implementation asself
from aninit
method.I'm not sure there really should be an expected designated initializer in Kotlin for either of these classes. This is likely an interop error. In Objective-C this compiles and I'm able to construct
[[MyNSInputStream alloc] init]
just fine:It works to use either the memory or URL constructor of both
NSInputStream
andNSOutputStream
. I figured the empty memory versions were likely to be the most lightweight. Is there some way to suppress this designated initializer requirement to call the no-arg constructor? It might also make sense to file an issue to fix the interop. Since we're overriding all ofNSInputStream
andNSOutputStream
's public API functions and not using anything from the superclass, I think this should be safe though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related issues:
https://youtrack.jetbrains.com/issue/KT-47992
JetBrains/kotlin-native#2010
Looks like there is a workaround for disabling in custom .def interop, but this won't work for the platform interop in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, seems like there's no way to inherit streams using no-args
init
ctor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I was mostly concerned about the possibility of inheriting some methods from NSInput/OutputStream subclasses instantiated for
initFromData
,initToMemory
. But it's definitely not the case, as there were errors due to non-implemented run-loop-related methods previously (so nothing was inherited).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the suggested doc comments. It would be strange for a consumer to call any of those
init
functions after receiving the instantiated stream object fromasNSInputStream()
orasNSOutputStream()
. In Kotlin it already shows an error trying to callinit
functions directly, not through a constructor. I don't think I've ever seen a call to aninit
function even in Objective-C that wasn't directly after a call toalloc
. Swift doesn't allow this either, whereinit
functions are only usable to construct objects now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the doc! Yes, it should be possible to call these initializers only from Objective-C code and it seems like nobody actually does that, but it is still possible :')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, but status transition diagrams seems to look like this:
Do all the transitions make sense? Should we change the status to
error
if it isclosed
? Should we fail and change the status toerror
when writing into a stream whose underlying sink was closed but the stream itself was not opened yet, or it's better to exit thewrite
immediately if the status isnot-open
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the stream is not open,
write()
will just return -1, but not change the status to error.NSStreamStatusError
is a result of a terminal error, so any error that is correctable, like calling a function in the wrong order or with bad parameters, should not change the status to error.I don't think we should go from
NSStreamStatusClosed
toNSStreamStatusError
. Callingwrite
on a closed platform stream just returns -1. I'll change that throwingIOException
to returning -1 when closed.We should probably mirror the behavior of the platform streams and not allow closing a stream that has never been opened. I'll make that change. You're missing the close from the
NSStreamStatusOpen
state though.Where are you getting the
NSStreamStatusOpen -->|error| NSStreamStatusError;
from? I believe we should only get anerror
duringwrite
. That's the only place it's being set.I guess we just pass right through
NSStreamStatusOpening
. Maybe it makes sense to set it right before setting toNSStreamStatusOpen
, in case some key-value observer is expecting it in Objective-C? We don't actually do anything to open the underlyingSink
orSource
, so setting both statuses is entirely to align withNSStream
expected behavior.Sinks don't provide a mechanism to be limited in capacity, so we never get
NSStreamStatusAtEnd
.This is what I believe the status lifecycle should look like after these changes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's possible when the stream is still open, but the underlying sink is closed and someone is calling
write
.Not saying that it's wrong or problematic, just mentioning the possible transition.
My understanding is that it's mainly intended for streams with complex opening protocols, like network connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok. This should be changed now where any
error
would come afterwrite
has changed status toNSStreamStatusWriting
.