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

Sketch for implementing sessions with flattened protocol. #109

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

powdercloud
Copy link
Contributor

Hi again! I made a rough sketch for the flattened protocol. It is really quite rough since I'm a novice with Go, and there are certainly some loose ends, especially around closing connections. I tried my best at keeping the integrity / design of your work intact. And upfront: thank you, I feel I learned quite a bit about how a well designed Go library looks!

The gist is, I got the unittest for the sessions in sessions_test.go to work with the flattened protocol, so this is why I figured I should post what I have thus far. If the approach in this PR is viable, I'm happy to work on fixing it / finishing it, or if you'd like to take this over that's fine too, or if some other way of getting flat sessions support makes more sense, please bring it on. Thank you. :-)

Approach taken here:

  • Trying to keep this compatible with existing code, so that upgrading to this version will "just work", but not switch to flattened protocol. To switch, minor tweaks in the client code are required (see session_test.go).
  • The non-flattened sessions create an rpcc.Conn instance which reads/writes to an underlying instance by sending Target.sendMessageToTarget etc. This is still the case; but to support non-flattened sessions this PR adds rpcc.SessionConn, which is much lighter weight - it carries the SessionID and can be closed, but the traffic is sent via it's parent connection, an instance of rpcc.Conn, which keeps track of its rpcc.SessionConn instances.
  • Various methods and structs now have a SessionID field / sessionID argument.
  • A few public method names are new, when I felt that it's better to keep the old one as is for compatibility. E.g., rpcc.Invoke (made rpcc.InvokeRPC with extra param), session.Manager.Dial (made session.Manager.Attach), and in the generated code there's cdp.Client.NewSession to get a client that talks via a specific session.

@codecov
Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #109 into master will decrease coverage by 6.2%.
The diff coverage is 52.5%.

@@           Coverage Diff            @@
##           master    #109     +/-   ##
========================================
- Coverage    90.6%   84.4%   -6.2%     
========================================
  Files          15      15             
  Lines         896    1020    +124     
========================================
+ Hits          812     861     +49     
- Misses         55     126     +71     
- Partials       29      33      +4

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.

1 participant