Fix JSON-RPC response streaming #6078

emlautarom1 · 2023-09-06T18:56:05Z

Fixes #5955

Changes

Properly stream JSON-RPC results using Ipc and WebSockets

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Notes on testing

Tests could be flaky on CI due the usage of Task.Delay for synchronization.
A specific test is marked as explicit due to long running time. On my machine, it must be run outside Rider, otherwise it crashes the IDE.

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

Remarks

During testing I found that a feature is not working properly (maxBatchResponseBodySize)

- Wrap a `WebSocket` supporting `Stream` operations

- Decorator that counts the bytes written

- Do not dispose `_socket` - When disposing, just send an "end of message"

- Instead of using 'SendRawAsync' operate on the socket using a 'Stream'

- Restructure tests (IPC, WebSockets)

- Test still could be flaky

- Do nothing on 'ReportCall' - Return default value on 'GetMethodStats' - Useful during testing

- Use delays to synchronize =(

- We already are using delays

emlautarom1 · 2023-09-06T19:00:18Z

src/Nethermind/Nethermind.JsonRpc/WebSockets/JsonRpcSocketsClient.cs

+            // ? What if we write more than int.MaxValue.
+            // Result could be negative
+            // return (int)resultData.WrittenBytes;
+            return (int)resultData.WrittenBytes;


Technically, we could send infinite number of bytes, but our result is bounded by int.MaxValue according to the result type of this operation.

The CounterStream uses a long internally, but this still could be problematic (we are bounded by long.MaxSize).

emlautarom1 · 2023-09-06T19:10:35Z

src/Nethermind/Nethermind.JsonRpc/WebSockets/JsonRpcSocketsClient.cs

                            _ = _jsonRpcLocalStats.ReportCall(entry.Report);

                            // We reached the limit and don't want to responded to more request in the batch
-                            if (!_jsonRpcContext.IsAuthenticated && singleResponseSize > _maxBatchResponseBodySize)
+                            if (!_jsonRpcContext.IsAuthenticated && resultData.WrittenBytes > _maxBatchResponseBodySize)


This feature is fundamentally broken. Consider the following scenarios where we are sending JsonRpcResults and each value in <> represents the number of bytes:

Scenario 1: [ <100>, <100>, <999999> ] (Collection) Scenario 2: { items: [ <999999>, <999999>, <999999>, ... ] } (Single)

On 1, if we limit set _maxBatchResponseBodySize to 300, that is, we limit the response size to 300 bytes, we will fully send the last element of the collection, sending a total of 100 + 100 + 9999999 bytes, which exceeds the limit.

On 2, since we're dealing with a single element, we do not perform any checks, despite the element containing a collection of items that could exceed any possible limit.

A proper solution would require the underlying stream (resultData) to stop as soon as the limit is reached, just by taking a look at the number of bytes written. A decorator like LimitedStream(Stream stream, long writeAtMostBytes) would suffice.

There are tests (ignored for now) that show how this feature is currently broken (see: https://github.com/NethermindEth/nethermind/pull/6078/files#diff-4fc7730d1718514a2e6903a9a4470be7f5809c327011ae97ff5a75f8e04d825eR127).

emlautarom1 · 2023-09-06T19:20:45Z

src/Nethermind/Nethermind.JsonRpc/WebSockets/JsonRpcSocketsClient.cs

-                    "Request was canceled due to enabled timeout.",
-                    result.Response.Id,
-                    result.Response.MethodName));
+                _jsonSerializer.SerializeWaitForEnumeration(dest, result.Response);


Despite Stream supporting async operations, internally we're using Newtonsoft.Json which will only use System.IO.Stream.Write(byte[] buffer, int offset, int count)

I dont think we're still using Newtonsoft.Json, after Ben's changes? right?

nvm I didnt notice PR merged xD

emlautarom1 · 2023-09-06T19:22:24Z

src/Nethermind/Nethermind.JsonRpc.Test/Sockets/WebSocketTests.cs

+            for (int i = 0; i < messageCount; i++)
+            {
+                await client.SendJsonRpcResult(result);
+                await Task.Delay(100);


I would really prefer a different approach to ensure that the "server" has finished processing all messages instead of relying on delays.

Use ManualResetEventSlim?

src/Nethermind/Nethermind.Sockets/CounterStream.cs

LukaszRozmej · 2023-09-07T11:25:17Z

src/Nethermind/Nethermind.Sockets/ISocketHandler.cs

@@ -15,5 +16,6 @@ public interface ISocketHandler : IDisposable
        Task SendRawAsync(ArraySegment<byte> data, bool endMessage = true);
        Task<ReceiveResult?> GetReceiveResult(ArraySegment<byte> buffer);
        Task CloseAsync(ReceiveResult? result);
+        Stream SendUsingStream();


GetUnderlyingStream?

I initially used a name like AsStream, but I found it to be confusing:

For Ipc, the NetworkStream does not own the underlying Socket, meaning that disposing this stream does not close the connection.

Same for WebSockets where on dispose we send a "end of message" message and it does not manage lifecycle events of the connection (ex. "close" messages).

For both cases, each Stream is intended to be used to send a single message during the lifetime o the underlying Socket. What do you think @LukaszRozmej?

src/Nethermind/Nethermind.Sockets/WebSocketStream.cs

LukaszRozmej · 2023-09-07T11:27:16Z

src/Nethermind/Nethermind.Sockets/WebSocketStream.cs

+
+public class WebSocketStream : Stream
+{
+    private WebSocket? _socket;


When it would be null?

LukaszRozmej · 2023-09-07T11:27:41Z

src/Nethermind/Nethermind.Sockets/WebSocketStream.cs

+        }
+        finally
+        {
+            _socket = null;


why nulling it?

The idea is to use a WebSocketStream for a single message, so we want to ensure that we're not using it to send a message after one has already been sent.

LukaszRozmej · 2023-09-07T11:28:03Z

src/Nethermind/Nethermind.Sockets/WebSocketStream.cs

+
+    private void ThrowIfDisposed()
+    {
+        if (_socket == null) throw new ObjectDisposedException(nameof(_socket));


maybe better to have just Disposed flag?

src/Nethermind/Nethermind.JsonRpc.Test/Sockets/RandomObject.cs

src/Nethermind/Nethermind.JsonRpc.Test/Sockets/RandomString.cs

src/Nethermind/Nethermind.JsonRpc.Test/Sockets/RandomStringArray.cs

LukaszRozmej · 2023-09-07T11:31:27Z

src/Nethermind/Nethermind.JsonRpc.Test/Sockets/WebSocketTests.cs

+            for (int i = 0; i < messageCount; i++)
+            {
+                await client.SendJsonRpcResult(result);
+                await Task.Delay(100);


Use ManualResetEventSlim?

- Remove 'RandomX' classes, prefer methods

- Might help with flaky tests on CI

- It actually has no effect on CI

fselmo · 2023-10-17T15:21:41Z

I'm curious if these changes are related to #6169. Every now and again, for newPendingTransactions, two messages come in as one and fails json decoding.

fselmo · 2024-01-25T16:46:51Z

I'm curious if these changes are related to #6169. Every now and again, for newPendingTransactions, two messages come in as one and fails json decoding.

I'm seeing the same issue now when I test against IPC streaming with subscriptions. Perhaps this is related once again. I opened issue #6614 describing the issue.

emlautarom1 added 30 commits September 1, 2023 16:06

Add WebSocketStream

02f9905

- Wrap a `WebSocket` supporting `Stream` operations

Add single test

22b5a90

Include possible fix notes

4589530

Introduce 'CounterStream'

7bf17d3

- Decorator that counts the bytes written

Do not take ownership of the underlying WebSocket

a46d19e

- Do not dispose `_socket` - When disposing, just send an "end of message"

Add 'SendUsingStream' to 'ISocketHandler'

08c0b55

- Instead of using 'SendRawAsync' operate on the socket using a 'Stream'

Add more tests

b899b3f

- Restructure tests (IPC, WebSockets)

Remove unused safe null operators

647d5c0

Replace implementation with 'Stream's

3ae91f3

Merge branch 'master' into fix/socket_streams

f7d1b49

Use Test annotations

0f6ee5f

Test 'WebSockets' impl

28f3741

Ensure server has finished without using 'Delay's

2a2a901

- Test still could be flaky

Add 'NullJsonRpcLocalStats'

aaeca89

- Do nothing on 'ReportCall' - Return default value on 'GetMethodStats' - Useful during testing

Initial WebSockets collection tests

1bcdc39

Rollback

fd906be

Fix test suite

57a74bd

- Use delays to synchronize =(

Remove manual synchronization

8d8c510

- We already are using delays

Refactor message counting for WebSockets

8e12e1f

Refactor IPC tests

3f61ed9

Test sending collections through IPC

edd2566

Increase test case count

b5f8d1b

Use 'NullJsonRpcLocalStats' when required

50e4273

Reintroduce streaming

5acb02e

Send collections using streams

30ca4f9

Invert if branches

2fee77b

Add tests for 'maxBatchResponseBodySize' option

8d43c89

Extract 'RandomString' into separate class

412f5f0

Extract random data generation

b19bbf3

Separate 'Ipc' from 'WebSocket' tests

4462a5d

emlautarom1 added 3 commits September 6, 2023 13:56

Reorder fields, rename server

0aa8903

Fix random object usages

cdcdb52

Merge branch 'master' into fix/socket_streams

6b59144

emlautarom1 commented Sep 6, 2023

View reviewed changes

Remove commented out line

ae2f097

emlautarom1 commented Sep 6, 2023

View reviewed changes

Formatting

8aa9c14

emlautarom1 requested review from asdacap, LukaszRozmej and dceleda September 6, 2023 19:58

asdacap reviewed Sep 7, 2023

View reviewed changes

src/Nethermind/Nethermind.Sockets/CounterStream.cs Show resolved Hide resolved

LukaszRozmej approved these changes Sep 7, 2023

View reviewed changes

emlautarom1 added 6 commits September 7, 2023 10:33

Move tests into single file

1deacd7

- Remove 'RandomX' classes, prefer methods

Throw 'InvalidOperationException' when socket is null

c663766

Formatting

e9482c8

Retry WebSockets tests

fef0597

- Might help with flaky tests on CI

Remove retries

65564b7

- It actually has no effect on CI

Merge branch 'master' into fix/socket_streams

c7e67cf

emlautarom1 merged commit fad0455 into master Sep 11, 2023
61 checks passed

emlautarom1 deleted the fix/socket_streams branch September 11, 2023 19:21

emlautarom1 mentioned this pull request Sep 14, 2023

[Unit Tests] CI - lots of flaky tests #3409

Closed

emlautarom1 mentioned this pull request Oct 20, 2023

Handle concurrent WebSockets messages #6210

Merged

16 tasks

emlautarom1 mentioned this pull request Nov 30, 2023

Improved Json Serialization #6152

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix JSON-RPC response streaming #6078

Fix JSON-RPC response streaming #6078

emlautarom1 commented Sep 6, 2023

emlautarom1 Sep 6, 2023

emlautarom1 Sep 6, 2023

emlautarom1 Sep 6, 2023

emlautarom1 Sep 6, 2023 •

edited

Loading

emlautarom1 Sep 6, 2023

Demuirgos Jan 31, 2024

Demuirgos Jan 31, 2024

emlautarom1 Sep 6, 2023

LukaszRozmej Sep 7, 2023

LukaszRozmej Sep 7, 2023

emlautarom1 Sep 7, 2023

LukaszRozmej Sep 7, 2023

LukaszRozmej Sep 7, 2023

emlautarom1 Sep 7, 2023

LukaszRozmej Sep 7, 2023

LukaszRozmej Sep 7, 2023

fselmo commented Oct 17, 2023

fselmo commented Jan 25, 2024

Fix JSON-RPC response streaming #6078

Fix JSON-RPC response streaming #6078

Conversation

emlautarom1 commented Sep 6, 2023

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

Remarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fselmo commented Oct 17, 2023

fselmo commented Jan 25, 2024

emlautarom1 Sep 6, 2023 •

edited

Loading