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

Refactor serialization code #93

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

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

@caleblloyd caleblloyd commented Oct 10, 2016

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

@bgrainger bgrainger commented Oct 10, 2016

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

@caleblloyd caleblloyd commented Oct 10, 2016

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

@bgrainger bgrainger commented Oct 10, 2016

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).

@caleblloyd caleblloyd mentioned this pull request Oct 12, 2016
@bgrainger bgrainger merged commit a25b2e5 into master Oct 12, 2016
4 checks passed
@bgrainger bgrainger deleted the refactor-serialization branch Oct 12, 2016
@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
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants