8000 Refactor serialization code by bgrainger · Pull Request #93 · mysql-net/MySqlConnector · GitHub
[go: up one dir, main page]
Skip to content

Refactor serialization code #93

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 12, 2016
Merged

Refactor serialization code #93

merged 3 commits into from
Oct 12, 2016

Conversation

bgrainger
Copy link
Member
@bgrainger bgrainger commented Oct 9, 2016 •
edited
Loading

This breaks PacketTransmitter up into multiple independent components:

  • IPayloadWriter: writes a MySQL command and its data to an underlying transmission layer
    • standard implementation breaks it up into 16MiB packets with increasing sequence numbers
  • IPacketWriter: serializes MySQL packets for transmission via a byte-oriented protocol
  • IByteWriter: writes a byte array to a destination
    • standard implementation provides sync and async Socket support
  • Reader versions of the three above interfaces, that invert their behaviour

Not only does this provide separation of concerns, it makes the "protocol stack" pluggable and should allow compression #31 and SSL #88 to be implemented much more easily.

This is a work-in-progress; pushed for early feedback. TODO:

  • squash bugfix commits
  • extract to one class per file
  • code documentation
  • decide on final namespaces
  • implement compression (to validate the implementation)
  • implement SSL proof-of-concept (to verify no major blocking issues)

@caleblloyd
Copy link
Contributor

The top level class that the driver interacts with should implement Stream or NetworkStream

This would allow for streams to be chained as Middleware. For example, if compression and SSL were enabled, stream call stack would look like:

Write: Caller.Write -> MySqlStream.Write -> MySqlCompressedStream.Write -> SslStream.Write -> the wire
Read: Caller.Read -> MySqlStream.Read -> MySqlCompressedStream.Read -> SslStream.Read -> the wire

@bgrainger
Copy link
Member Author

I decided against that for the following reasons:

  • The dichotomy between Read/ReadAsync and Write/WriteAsync is unhelpful for this project; it's proved to be more useful (so far) to use one method with IOBehavior
  • Stream doesn't permit the use of ValueTask
  • Stream has a lot of other functionality that is unneeded; it would be unclear which parts of Stream were necessary to override (or could be called)
  • See other downsides of Stream here: https://github.com/davidfowl/channels
    • Avoiding Stream might make it easier to port to Channels in the future (low priority reason)
  • If it would be useful to use a Stream, it should be possible to write an IByteWriter adapter that is implemented with a Stream, allowing a Stream to be easily plugged in as middleware

@caleblloyd
Copy link
Contributor

I decided against that for the following reasons:

Another reason against it: In my testing, SocketAwaitable was faster than using async functions from the NetworkStream coming from TcpClient.GetStream(). Maybe SocketAwaitable does a better job at avoiding copying?

If it would be useful to use a Stream, it should be possible to write an IByteWriter adapter that is implemented with a Stream, allowing a Stream to be easily plugged in as middleware

This is probably what we'll need to do for SSL then.

The Compression piece looks to be MSQL Specific - on second thought, we can't just pipe the whole payload through a GZip stream. Each MySql packet has to be compressed conditionally.

@bgrainger
Copy link
Member Author

Maybe SocketAwaitable does a better job at avoiding copying?

It certainly does a lot less allocation; see Stephen Toub's writeup here: https://blogs.msdn.microsoft.com/pfxteam/2011/12/15/awaiting-socket-operations/

Each MySql packet has to be compressed conditionally.

This was also part of the reason behind the separation into three separate interfaces: a CompressingPacketWriter should be able to send its output to an IPayloadWriter which will wrap the standard MySQL packet header around it (because the first four bytes in a regular and a compressed packet are the same).

@bgrainger bgrainger force-pushed the refactor-serialization branch from 8f9890b to 7655c60 Compare October 12, 2016 04:56
8000
@bgrainger bgrainger force-pushed the refactor-serialization branch from 7655c60 to bdb6be7 Compare October 12, 2016 05:59
@bgrainger bgrainger mentioned this pull request Oct 12, 2016
Closed
@bgrainger bgrainger force-pushed the refactor-serialization branch from 7c52a5e to a25b2e5 Compare October 12, 2016 06:58
@caleblloyd caleblloyd mentioned this pull request Oct 12, 2016
Closed
@bgrainger bgrainger merged commit a25b2e5 into master Oct 12, 2016
@bgrainger bgrainger deleted the refactor-serialization branch October 12, 2016 14:23
@bgrainger bgrainger changed the title WIP: Refactor serialization Refactor serialization code Oct 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bgrainger @caleblloyd
0