Skip to content
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

Multiple network stream support #986

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

d4l3k
Copy link

@d4l3k d4l3k commented Jul 11, 2018

This adds the basic support for multiple network streams via a Network::MultiplexerStream. It aggregates the diffs for multiple child streams into one.

This is intermediate work towards implementing #337.

@brainstorm
Copy link

Can someome @mobile-shell review this please? @cgull ?

@andersk
Copy link
Member

andersk commented Sep 21, 2018

This didn’t pass the automated CI test, because we still support platforms with poor or missing C++11 support (that may change after this release, but for now it’s a requirement). When building on a C++11 system, make check shows a failing test. Also, some quick manual tests show this isn’t backwards compatible with the existing Mosh protocol in either direction.

@d4l3k
Copy link
Author

d4l3k commented Sep 21, 2018

@andersk I realize the tests are failing, just wanted someone to give a thumbs up/thumbs down whether this approach would ever be accepted before fixing it up.

Does it need to be backwards compatible between versions? I can probably hack some support in if necessary.

@keithw
Copy link
Member

keithw commented Sep 24, 2018

We haven't broken compatibility on the wire protocol since early 2012 and I'm not too enthusiastic to do it now -- so yeah, any protocol design here would probably need to be backwards compatible (or bundled with some huge major rev). You've probably seen the amount of anger we induced by turning down a backwards-compatible protocol change that added support for ssh-agent forwarding, so I just want to be upfront that we're unlikely to take major protocol changes to the Mosh/SSP protocol that haven't been discussed and reached consensus on the mailing lists.

@brainstorm
Copy link

Keith, thanks for your answer. I've quickly scanned the mosh-devel ML archives up to the beginning of 2017 but couldn't easily find that thread/discussion... can you brief us on what would be an acceptable patch would look like if even backwards-compatible changes are (routinely?) rejected?

I know that the stakes are high for such a popular, stable and great project, but I just want to manage expectations on whether we'll likely see this feature landing the main codebase at all or we shouldn't even be trying at this point?

Perhaps there's some major rearchitecting going on for future major releases and this feature makes more sense there instead of trying to merge it now?

@keithw
Copy link
Member

keithw commented Sep 25, 2018

Hello -- I appreciate your candor here. I was thinking of the discussions around #120 (and on the mailing list, https://www.mail-archive.com/[email protected]/msg00980.html).

I think something like this could be part of a "mosh v2" (and I also have my ideas about what might be nice to have in "mosh v2"), but we honestly haven't even started the process of talking about what might be in scope for that. Probably we might start by brainstorming what we really want to do here, and in what dimensions we want to improve on mosh v1, and then build the protocol around that. That would be my druthers for the way to go, but it's a big effort not yet begun... It's not like we've started to rearchitect.

@brainstorm
Copy link

It's almost a year later now, is there a mosh v2 in sight where this feature could be included, @keithw? The bounty for this feature getting implemented keeps increasing: #337 (comment) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants