Skip to content

Conversation

@DSilence
Copy link
Contributor

Implemented support for new DocumentWriter for websocket transport.
Things to note:

  1. Writing to socket is achieved by using a proxy stream object - WebsocketWriterStream. It solves several issues:

    • The initial idea of DocumentWriter method has a memory limit of 1 mb per message

    • There is some overly complex code around buffers in DocumentWriter implementation. Reducing it to a single method that writes to stream is great.
      There is a con, however. There is always a need to call the websocket API at lease twice. Not sure how this will affect the performance and not sure how to test this (perhaps, some load test?).

  2. The ProtocolMessageListener was sending a message.Id twice, once as part of ExecutionResult, and once in OperationMessage. I believe this is not requred, but I may be wrong.

Let me know what you think! Should this approach be accepted, it should be possible to remove the "extra" method within the DocumentWriter, and always stick to Stream API for serialization.

Copy link
Member

@johnrutherford johnrutherford left a comment

Choose a reason for hiding this comment

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

I think we should add unit tests to ensure data is written successfully.

Copy link
Contributor

@pekkah pekkah left a comment

Choose a reason for hiding this comment

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

Some tests for the writing would be nice. Also don't really get why the custom stream is required...

@johnrutherford
Copy link
Member

@pekkah, the custom stream is required to be able to write to the websocket in chunks from a buffer. The alternative is to serialize the whole response to memory and then write the whole byte array at once, but this uses more RAM.

@DSilence
Copy link
Contributor Author

Added unit tests for payloads of varying size (from several bytes, to 100 mb).

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