Skip to content

Fix data issues when spanning >16k bytes#16

Merged
quinchs merged 2 commits intomasterfrom
fix/packet-span
Jul 25, 2023
Merged

Fix data issues when spanning >16k bytes#16
quinchs merged 2 commits intomasterfrom
fix/packet-span

Conversation

@quinchs
Copy link
Copy Markdown
Contributor

@quinchs quinchs commented Jul 24, 2023

Summary

Netty transfers data to the binding in chunks of <=16k bytes at a time, the EdgeDB protocol can have messages with lengths over that size, therefor the binding must collect the chunks to build a complete message if the protocol message is >16k bytes.

The binding did attempt to do this with a contract structure, creating PacketContracts for messages that can't be read with the provided buffer, ref:

https://github.com/edgedb/edgedb-java/blob/abb7f8a797e0d2ec21c0ee4ba298022fa2cf06a9/src/driver/src/main/java/com/edgedb/driver/binary/PacketSerializer.java#L77-L98

although, this code failed to account for successful contracts with remaining data, ignoring it and misreading the protocol data.

This PR fixes that by correctly consuming the remaining data of a completed contract, as well as correctly handling cases where multiple chunks are sent for one large data message.

@quinchs
Copy link
Copy Markdown
Contributor Author

quinchs commented Jul 24, 2023

Fixes #15

@quinchs quinchs requested review from nsidnev and scotttrinh July 24, 2023 20:35
@scotttrinh
Copy link
Copy Markdown
Contributor

I'm not familiar enough with how the Contract pattern works here to comment on the actual logic, but it would be helpful to see a test that exercised this 16k message with something simple like a select on a 16k string full of a or something obvious like that. Is that straightforward to add?

@quinchs
Copy link
Copy Markdown
Contributor Author

quinchs commented Jul 24, 2023

Added tests for both multiple data packets spanning >16k and one data packet with a size over >16k

Copy link
Copy Markdown
Contributor

@scotttrinh scotttrinh left a comment

Choose a reason for hiding this comment

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

Tests make sense, thanks for checking both cases! 🏆

@quinchs quinchs merged commit 40eb644 into master Jul 25, 2023
@quinchs quinchs deleted the fix/packet-span branch July 25, 2023 17:06
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.

2 participants