Alan tries using a socket Sink #97

cryptoquick · 2021-03-26T13:26:23Z

This is a personal story, but I've done my best to switch things up so it's less personal, while also doing my best to keep the details more grounded in the specifics. I also tried to make it follow a similar format to the others.

Hopefully the "drama" portion isn't too distracting, but I think it's important to explain what happened, because we need to acknowledge the issue and be honest about it.

Of course, I'm open to suggestions. I also will admit, I had trouble working on the second example. I'm still not sure what the best solution is with that approach.

cryptoquick · 2021-03-26T14:03:57Z

Also, I just realized, I was supposed to use an existing persona. I'll update this, give me a few. I originally chose the name Carrie just because of... Well, personal reasons.

cryptoquick · 2021-03-26T14:20:02Z

I've updated this, and I'd like feedback on either echo examples, as well as suggestions for the FAQ section.

ibraheemdev

Just a few minor nits. Overall, I think this is really well done, yet sad at the same time because I've had a similar experience.

src/vision/status_quo/carrie_tries_a_socket_sink.md

ibraheemdev · 2021-03-26T14:26:51Z

src/vision/status_quo/carrie_tries_a_socket_sink.md

+
+Alan wonders if he's thinking about it wrong, but the solution isn't as obvious as his earlier Sink approach. Looking around, he realizes a solution to his problems already exists-- as others have been in his shoes before-- within two other nearly-identical pull requests, but they were both closed by the project maintainers. He tries opening a third one with the same code, pointing to an example where it was actually found to be useful. To his joy, his original approach works with the code in the closed pull requests in his local copy! Alan's branch is able to compile for the first time.
+
+However, almost immediately, his request was also shut down, and the maintainer suggested he try the complex, unobvious, and unspecific solution that he had already tried and just couldn't get it to work.


Suggested change

However, almost immediately, his request was also shut down, and the maintainer suggested he try the complex, unobvious, and unspecific solution that he had already tried and just couldn't get it to work.

However, his pull request gets shut down instantaneously, and the maintainer suggests he try the complex, unobvious, and unspecific solution that he had already tried and couldn't get it to work.

I think instantaneously is the wrong phrase here, but the other suggestions are great!

src/vision/status_quo/carrie_tries_a_socket_sink.md

Co-authored-by: Ibraheem Ahmed <[email protected]>

cryptoquick · 2021-03-26T14:41:54Z

@ibraheemdev Really good suggestions, thank you! And I appreciate the support.

jbr · 2021-03-26T17:10:04Z

The other side of this story is relevant to this wg.

The short version is that because the futures crate is treated as if it were stdlib, not implementing traits from that library is seen as breaking from convention. If the futures crate were to deprecate Sink (or alternatively if less likely for there to be action on moving Sink into std and therefore async-std), this would have been easier to navigate as a framework author. The awkward implementation of the code in question was explicitly intended to take a Sink based api and hide the Sink trait. The alternative was to fork async-tungstenite and make the changes there.

Summary: The futures crate is treated like std and Sink is seen as comparably std-track as Stream and Future and there is nothing to reference as a library author that indicates whether this is or is not the case

Edit: At least one of the maintainers in the above story has nothing against Sink and is entirely neutral about its importance in the ecosystem, but sees the futures crate as a historical artifact

jbr · 2021-03-26T17:34:46Z

@cryptoquick rereading the code in this story: The connection is Clone, so there's no reason to re-wrap it with an Arc Mutex. You can just clone it as many times as you want and do whatever you'd do with a stream or a sink.

For other readers: I'm sorry this part spilled over here. The relevant bit is that "implement Sink" was the feature request, not "implement Clone"

nikomatsakis

Ack! I ran out of time. I'm going to read this over. I don't have a problem with including some amount of drama, and I would even say it's good to include some element of that, since I think it's something of an Elephant in the Room so much of the time, but I also think we should be careful how we phrase it. I haven't really read that part of the text closely enough to make any suggestions, I will attempt to do so later.

nikomatsakis · 2021-03-26T18:00:33Z

src/vision/status_quo/alan_tries_a_socket_sink.md

+
+One of the goals is to switch from a WebSocket implementation using raw TCP sockets to one managed behind an HTTP server library, so both HTTP and WebSocket commands can be forwarded to a transport-agnostic RPC server. He finds an HTTP server that's similar to one he's used to using with the other async framework, and a WebSocket middleware library that goes with it.
+
+However, as he's working, Alan encounters a situation where the socket needs to be written to within an async thread, and the traits just aren't working. He wants to split the stream into a sender and receiver:


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 a Stream + Sink. Is the reason that this code doesn't work because ws_stream does not implement Sink? That could be made clearer -- for example, you could show the error message he gets.

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.

Where is this read lock? Inside the socket code?

OK, I think maybe I'm starting to see. From reading the code snippet, what I get is this:

He wants to have incoming messages and then spawn off threads that will do some RPC before forwarding the messages on to another place?

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?

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.

Also, I'm totally for improvements to tide-websockets, but is that not outside the scope of this PR?

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

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.

Definitely changes to tide-websockets or whatever are not in scope for the story.

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 the WebSocket type that implements futures::Sink and futures::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 the Sink trait as is the correct design for this (nor am I convinced of the opposite; just neutral).

eminence · 2021-03-27T01:49:58Z

src/vision/status_quo/alan_tries_a_socket_sink.md

+
+*Here are some standard FAQ to get you started. Feel free to add more!*
+
+* **What are the morals of the story?**


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:

but is frustrated to find that the Sink trait wasn't implemented in the WebSockets middleware library he's using.

lamenting the popularity of Sink in the Rust ecosystem, and how it's complex, bad and not worth it.

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.

I've added some more, do you think that helps?

Certainly having core abstractions in stdlib would help here.

eminence · 2021-03-27T02:08:26Z

src/vision/status_quo/alan_tries_a_socket_sink.md

+
+Alan wonders if he's thinking about it wrong, but the solution isn't as obvious as his earlier `Sink` approach. Looking around, he realizes a solution to his problems already exists-- as others have been in his shoes before-- within two other nearly-identical pull requests, but they were both closed by the project maintainers. He tries opening a third one with the same code, pointing to an example where it was actually found to be useful. To his joy, his original approach works with the code in the closed pull requests in his local copy! Alan's branch is able to compile for the first time.
+
+However, almost immediately, his request was shut down like the others, and the maintainer suggests he try the complex, unobvious, and unspecific solution that he had already tried and just couldn't get it to work.


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?

That would indeed be very helpful

That was what the second example was for, I'm not sure if there's a better way to explain that?

nikomatsakis

Hi @cryptoquick! I left various comments with suggested alterations to the wording. My goal is to keep the sense of your original story, but in general to avoid ascribing intentions or motivations to other people. I think a good goal should be to keep your description of other's actions short and factual -- particularly negative ones -- and instead ascribe feelings and interpretations to Alan.

For example, rather than saying "the maintainer sent a dismissive reply", I wrote "Alan felt dismissed".

I have to say that i would also really like to see the technical aspects of this story increased. I've had some confusion over the role of Sink myself, and I think it'd be helpful to dig into. I left some comments to that effect already, and I added a few more here. Maybe it'd be nice to just open a different story about Sink though.

src/vision/status_quo/alan_tries_a_socket_sink.md

Co-authored-by: Niko Matsakis <[email protected]>

cryptoquick · 2021-03-31T16:44:33Z

@eminence @nikomatsakis I've made a few more changes in response to your suggestions, can you take another look?

Also, I'm very grateful for your edits, @nikomatsakis, they were very helpful!

nikomatsakis

Ack! Apparently I never posted these comments, @cryptoquick, apologies. Let me give it a more thorough read.

src/vision/status_quo/alan_tries_a_socket_sink.md

nikomatsakis · 2021-03-31T20:55:57Z

src/vision/status_quo/alan_tries_a_socket_sink.md

+
+One of the goals is to switch from a WebSocket implementation using raw TCP sockets to one managed behind an HTTP server library, so both HTTP and WebSocket commands can be forwarded to a transport-agnostic RPC server. He finds an HTTP server that's similar to one he's used to using with the other async framework, and a WebSocket middleware library that goes with it.
+
+However, as he's working, Alan encounters a situation where the socket needs to be written to within an async thread, and the traits just aren't working. He wants to split the stream into a sender and receiver:


Where is this read lock? Inside the socket code?

nikomatsakis · 2021-03-31T20:57:13Z

src/vision/status_quo/alan_tries_a_socket_sink.md

+
+*Here are some standard FAQ to get you started. Feel free to add more!*
+
+* **What are the morals of the story?**


Certainly having core abstractions in stdlib would help here.

nikomatsakis

OK, I gave more of a read. @cryptoquick I think I'm starting to get what's going on, but I'm wondering if you and I could arrange a time to dig into the technical details synchronously? I'd really like to understand the point you're making here more deeply but I just don't quite see it yet!

Maybe ping me on discord or zulip? Or send me an email at [email protected] with your availability?

src/vision/status_quo/alan_tries_a_socket_sink.md

nikomatsakis · 2021-04-05T10:24:07Z

src/vision/status_quo/alan_tries_a_socket_sink.md

+
+One of the goals is to switch from a WebSocket implementation using raw TCP sockets to one managed behind an HTTP server library, so both HTTP and WebSocket commands can be forwarded to a transport-agnostic RPC server. He finds an HTTP server that's similar to one he's used to using with the other async framework, and a WebSocket middleware library that goes with it.
+
+However, as he's working, Alan encounters a situation where the socket needs to be written to within an async thread, and the traits just aren't working. He wants to split the stream into a sender and receiver:


OK, I think maybe I'm starting to see. From reading the code snippet, what I get is this:

He wants to have incoming messages and then spawn off threads that will do some RPC before forwarding the messages on to another place?

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?

src/vision/status_quo/alan_tries_a_socket_sink.md

nikomatsakis · 2021-04-05T10:26:47Z

src/vision/status_quo/alan_tries_a_socket_sink.md

+let ws_stream = Arc::new(Mutex::new(ws_stream));
+let poller_ws_stream = ws_stream.clone();
+
+async_std::task::spawn(async move {


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?

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.

cryptoquick · 2021-04-05T21:43:17Z

Great feedback so far! I've pushed some more changes. I'm not sure if GitHub has some comment threads that are outdated, so please double-check.

nikomatsakis

One rewording, otherwise, I think it's time to merge this.

src/vision/status_quo/alan_tries_a_socket_sink.md

cryptoquick · 2021-04-12T00:31:09Z

Awesome! I like your last change, it helps. All LGTM!

cryptoquick · 2021-05-03T23:25:50Z

@cryptoquick rereading the code in this story: The connection is Clone, so there's no reason to re-wrap it with an Arc Mutex. You can just clone it as many times as you want and do whatever you'd do with a stream or a sink.

For other readers: I'm sorry this part spilled over here. The relevant bit is that "implement Sink" was the feature request, not "implement Clone"

I just got around to exploring this, and you're correct! I totally missed that trait. I think I'll contribute an update to this story with the changes reflected it this PR, and archive my fork with an explanation of how to use tide-websockets properly. It also has the advantage that it's much cleaner, and probably performs better, too! Fewer locks and atomic references.

ChainSafe/forest#1089

I would have appreciated if this could have been pointed out earlier, however. It sure would have saved a lot of consternation. Did it take you a while to come to clarity on this? Even @yoshuawuyts and @skade never mentioned that solution. Admittedly, I did come off as a jerk, so I'll be forgiving.

jbr · 2021-05-03T23:57:08Z

this is probably OT for the current repo, but I think we didn't get to a solution earlier on this issue because it was presented as a demand to implement a specific trait, instead of "how do I accomplish {goal}," and sending a link to another application is expecting an open source maintainer read your code in order to understand what you're actually trying to do. It wasn't obvious why you thought you needed Sink, and starting a conversation by reopening asked and answered subjects didn't predispose anyone to put in the extra effort beyond "we're still not adding Sink."

cryptoquick · 2021-05-04T00:05:47Z

Yeah, that's valid. In my defense, I would say "it's hard to know what you don't know."

nikomatsakis · 2021-05-04T19:56:46Z

Well, I'm glad we found a good resolution in any case! <3

status quo: Carrie tries using a socket Sink

535807e

Switch to Alan.

f2192c4

cryptoquick changed the title ~~Add a status quo story about how Carrie tries using a socket Sink~~ Add a status quo story about how Alan tries using a socket Sink Mar 26, 2021

ibraheemdev suggested changes Mar 26, 2021

View reviewed changes

cryptoquick and others added 7 commits March 26, 2021 08:38

Update src/vision/status_quo/carrie_tries_a_socket_sink.md

fa2032f

Co-authored-by: Ibraheem Ahmed <[email protected]>

Update src/vision/status_quo/carrie_tries_a_socket_sink.md

83bdc33

Co-authored-by: Ibraheem Ahmed <[email protected]>

Update src/vision/status_quo/carrie_tries_a_socket_sink.md

d7c7489

Co-authored-by: Ibraheem Ahmed <[email protected]>

Update src/vision/status_quo/carrie_tries_a_socket_sink.md

2548488

Co-authored-by: Ibraheem Ahmed <[email protected]>

Update src/vision/status_quo/carrie_tries_a_socket_sink.md

2e85030

Co-authored-by: Ibraheem Ahmed <[email protected]>

Update src/vision/status_quo/carrie_tries_a_socket_sink.md

00073cb

Co-authored-by: Ibraheem Ahmed <[email protected]>

Update src/vision/status_quo/carrie_tries_a_socket_sink.md

9a41de3

Co-authored-by: Ibraheem Ahmed <[email protected]>

cryptoquick added 3 commits March 26, 2021 08:43

Move the blog post into the story sources section.

3f65592

Better phrasing, as suggested by @ibraheemdev.

dd7d632

Rename file.

15f2f7e

nikomatsakis reviewed Mar 26, 2021

View reviewed changes

eminence reviewed Mar 27, 2021

View reviewed changes

nikomatsakis reviewed Mar 31, 2021

View reviewed changes

cryptoquick and others added 4 commits March 31, 2021 10:21

Apply suggestions from code review

8686828

Co-authored-by: Niko Matsakis <[email protected]>

Add more specifics.

abb5e7b

Add more links and improve examples.

5e5cacc

Add interop morals as per @eminence's suggestion

f27c77e

nikomatsakis reviewed Apr 5, 2021

View reviewed changes

nikomatsakis requested changes Apr 5, 2021

View reviewed changes

Additional changes based on feedback.

d97472e

Add clarification of the task

2d51fe4

cryptoquick changed the title ~~Add a status quo story about how Alan tries using a socket Sink~~ Alan tries using a socket Sink Apr 5, 2021

cryptoquick added 2 commits April 5, 2021 16:47

Add rpc_ws_handler / WebSocketConnection function signature for context.

2857f1a

lol whoops

1b66a32

nikomatsakis added the status-quo-story-ideas "Status quo" user story ideas label Apr 6, 2021

nikomatsakis reviewed Apr 11, 2021

View reviewed changes

src/vision/status_quo/alan_tries_a_socket_sink.md Outdated Show resolved Hide resolved

Update src/vision/status_quo/alan_tries_a_socket_sink.md

c741cce

nikomatsakis approved these changes Apr 11, 2021

View reviewed changes

nikomatsakis merged commit 6f70b71 into rust-lang:master Apr 12, 2021

cryptoquick deleted the carrie-tries-a-socket-sink branch April 12, 2021 13:42

cryptoquick mentioned this pull request May 3, 2021

Remove tide-websockets-sink fork ChainSafe/forest#1089

Merged


		Alan wonders if he's thinking about it wrong, but the solution isn't as obvious as his earlier Sink approach. Looking around, he realizes a solution to his problems already exists-- as others have been in his shoes before-- within two other nearly-identical pull requests, but they were both closed by the project maintainers. He tries opening a third one with the same code, pointing to an example where it was actually found to be useful. To his joy, his original approach works with the code in the closed pull requests in his local copy! Alan's branch is able to compile for the first time.

		However, almost immediately, his request was also shut down, and the maintainer suggested he try the complex, unobvious, and unspecific solution that he had already tried and just couldn't get it to work.

	However, almost immediately, his request was also shut down, and the maintainer suggested he try the complex, unobvious, and unspecific solution that he had already tried and just couldn't get it to work.
	However, his pull request gets shut down instantaneously, and the maintainer suggests he try the complex, unobvious, and unspecific solution that he had already tried and couldn't get it to work.


		One of the goals is to switch from a WebSocket implementation using raw TCP sockets to one managed behind an HTTP server library, so both HTTP and WebSocket commands can be forwarded to a transport-agnostic RPC server. He finds an HTTP server that's similar to one he's used to using with the other async framework, and a WebSocket middleware library that goes with it.

		However, as he's working, Alan encounters a situation where the socket needs to be written to within an async thread, and the traits just aren't working. He wants to split the stream into a sender and receiver:


		Here are some standard FAQ to get you started. Feel free to add more!

		* What are the morals of the story?

Alan tries using a socket Sink #97

Alan tries using a socket Sink #97

Conversation

cryptoquick commented Mar 26, 2021

cryptoquick commented Mar 26, 2021

cryptoquick commented Mar 26, 2021

ibraheemdev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoquick commented Mar 26, 2021

jbr commented Mar 26, 2021 • edited Loading

jbr commented Mar 26, 2021 • edited Loading

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoquick Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbr Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis Apr 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

cryptoquick commented Mar 31, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoquick commented Apr 5, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

cryptoquick commented Apr 12, 2021

cryptoquick commented May 3, 2021

jbr commented May 3, 2021

cryptoquick commented May 4, 2021 • edited Loading

nikomatsakis commented May 4, 2021

ibraheemdev left a comment •

edited

Loading

jbr commented Mar 26, 2021 •

edited

Loading

jbr commented Mar 26, 2021 •

edited

Loading

cryptoquick Mar 31, 2021 •

edited

Loading

jbr Apr 6, 2021 •

edited

Loading

nikomatsakis Apr 11, 2021 •

edited

Loading

cryptoquick commented May 4, 2021 •

edited

Loading