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
Alan tries using a socket Sink #97
Alan tries using a socket Sink #97
Changes from 12 commits
535807e
f2192c4
fa2032f
83bdc33
d7c7489
2548488
2e85030
00073cb
9a41de3
3f65592
dd7d632
15f2f7e
8686828
abb5e7b
5e5cacc
f27c77e
d97472e
2d51fe4
2857f1a
1b66a32
c741cce
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 having a hard time understanding this, actually, there seems to be some context I am missing. Poking at the rustdoc, I see the
split
method, that works on aStream + Sink
. Is the reason that this code doesn't work becausews_stream
does not implementSink
? That could be made clearer -- for example, you could show the error message he gets.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.
It's not really an error message so much as undesired locking behavior, and by that I mean, data can't be sent because there's a read lock. And an RwLock doesn't work in this scenario, because they're both iterables, which are mutable, and so everything would have to request a write lock.
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.
Where is this read lock? Inside the socket code?
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.
OK, I think maybe I'm starting to see. From reading the code snippet, what I get is this:
I think what's confusing to me is that I think of a stream as an "async iterator", and I don't quite get how it could be split. It seems like what you want is to "splice" yourself in the middle of the stream?
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.
Yep! I think
async iterator
is a good way to think of a stream as long as it's coupled with the perspective that new items can be added to the stream, I don't think it has to be finite. It's kinda like a duplex unbounded MPSC. But streams vary a great deal on their implementation details, such as high water mark, and backpressure signalling, but that's a little beyond the scope of this.That said, I'm coming at this from a NodeJS perspective, where stream use is common, and often preferred. A WebSocket is an ideal candidate for this, but it's important it can be both read and written to in exclusion.
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.
Also, I'm totally for improvements to
tide-websockets
, but is that not outside the scope of this PR?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.
It's worth figuring out if the Sink trait is specifically necessary, since that's what this issue and the closed issue were about. The issue that I believe is relevant to the async wg is whether Sink should be standardized, but if we can satisfy this use case without needing Sink, that seems worth attaching to this story.
The latter situation I described where any clone could act as sender or receiver wouldn't require an improvement, it's the current state of the code. The sender and receiver are locked internally, so there can only be one clone sending at a time, and one clone polling the stream at a time, but they can be sending and receiving concomitantly
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 think the splicing was a misunderstanding on my part. I eventually realized that what was happening was just receiving messages, doing some processing, and responding in kind.
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.
Definitely changes to tide-websockets or whatever are not in scope for the story.
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 don't think the story itself should describe whether this can be solved without
Sink
, but I would definitely love to explore possible resolutions to this story as "shiny futures", or possibly as FAQs. I think we can merge without that. I talked to @cryptoquick some -- one thing that I was explaining is that it should be possible for him to add a newtype wrapped around theWebSocket
type that implementsfutures::Sink
andfutures::Stream
, which would then allow him to write his original example without having to maintain a complete parallel fork of the crate. This seems better to me.It does seem to me that some trait that means "can consume values of type
Item
" would be appropriate. I am not convinced that theSink
trait as is the correct design for this (nor am I convinced of the opposite; just neutral).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 starting to see the idea here, but it is indeed still confusing to me. this snippet of code doesn't seem to be starting with the same "general inputs". Maybe it'd be helpful if we could give a bit of an architectural diagram?
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.
The only input that matters (and is common between both examples) is
ws_stream
. The implementation details differ, but I think they're roughly equivalent in practice.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 it's not too long, is it possible to have a snippet of code that shows the code that Alan finds complex/unobvious/unspecific?
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 would indeed be very helpful
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 was what the second example was for, I'm not sure if there's a better way to explain that?
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 whole story is obviously about a real and specific event, which is great (we like specifics in these kind of stories).
But as I'm coming to the end of the story and thinking about morals, I'm also trying to draw generalizations too. Based on these comments:
I'm wondering if there is also something to say here about the difficulties/friction in trying to get multiple async libraries to interop, especially when they have conflicting design philosophies.
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've added some more, do you think that helps?
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.
Certainly having core abstractions in stdlib would help here.