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

Initial try for connection restart #10941

Closed

Conversation

skyline75489
Copy link
Collaborator

Summary of the Pull Request

This targets #3726 & #4379

Surprisingly, it works.

References

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@skyline75489
Copy link
Collaborator Author

/cc @zadjii-msft. WDYT? Does this looks like the correct way?

void ConptyConnection::Restart()
try
{
THROW_IF_FAILED(_LaunchAttachedClient());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to kill the client before creating a new one?

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So! In this case, I think it would be better for us to delete the old connection object and create a new one. That will simplify the Connection interface (not all connections can be "restarted"), and you will not have to ever handle a "backwards" state transition.

ITerminalConnection should represent the things that any connection could do, even after we get a plug-in model. If we can implement restart on top of "stop, delete, create new, start" we can free plugin-in authors from having to think about tracking any of their local state across multiple different sessions, etc.

What do you think about that?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 13, 2021
@zadjii-msft
Copy link
Member

FWIW I agree with Dustin on this one. I think stashing the ValueSet for the connection settings should be an easy enough way of keeping all the information relevant to starting a new clone of the connection.

@skyline75489
Copy link
Collaborator Author

Yeah you guys are right. This PR is just a demonstration.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 18, 2021
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