Ship threaded send

[huangminghuang]

Change Description

This PR

• creates additional threads to send data to ship clients.
• fixes a bug where state history plugin would send blocks with block_num smaller than the client requests when the state history block head is behind the block head of the client.
• stops the application when it fails to listen on the configured state history port.

Select ONE:

• Documentation

• Stability bug fix

• Other

• Other - special case

Testing Changes

Select ANY that apply:

• New Tests

• Existing Tests

• Test Framework

• CI System

• Other

Consensus Changes

• Consensus Changes

API Changes

• API Changes

Documentation Additions

• Documentation Additions

[heifner]

io_context per connection seems a bit heavy. Since most/all uses of SHiP are likely to have only a few connections it is likely ok. Could have one io_context and share it across all connections. Just FYI, I'm ok with leaving it as is.

[heifner]

I'm pretty sure this is not thread safe. You are not allowed to access a socket from more than one thread at a time. Here you have the main thread doing an async_read and the internal thread doing an write.

[huangminghuang]

That's not what I understand about socket programming. According to https://stackoverflow.com/questions/7418093/use-one-socket-in-two-threads, it should be safe for TCP. AFAIK, websocket is full-duplex communication channels over a single TCP connection, it should also be safe for read/write from different threads.

[heifner]

See thread-safety at the bottom of the page: https://www.boost.org/doc/libs/1_76_0/doc/html/boost_asio/reference/ip__tcp/socket.html
Also:
https://stackoverflow.com/questions/7362894/boostasiosocket-thread-safety

[spoonincode]

If you really want to do it, you can dup() the fd and create two boost sockets referring to the same underlying TCP connection. Then use one boost socket for read, one for write, on two separate threads. Not that i'm recommending doing that here, but that's how it can be done.

[spoonincode]

In case anyone finds this: my brain was thinking asio socket. The semantics of dup()ing a socket backed by a websocket::stream are almost certainly no good!

[heifner]

Seems like we should at least log that we are ignoring this send.

[huangminghuang]

The exception is reported via post at the end of thread function.

[spoonincode]

Seems racy. send_thread_has_exception won't be set true until some period of time after the io_context has stopped running. So it seems possible for send() to queue up another post() that will never be dispatched. If that's not a problem (and it might not be, I'm not sure on cursory look), then I don't see why this check needs to be here in the first place.

[huangminghuang]

The loop in the thread will break when the first task throw an exception, all the subsequent tasks would not be able to be executed. The check here is not strictly needed, just intended to be save some cycles.

[spoonincode]

I would like to review this further so I'm just putting this in for now to block merging. In particular I'm concerned about the change to a sync write()

[spoonincode]

The sync write was already a significant shut down concern, but I get the impression that the mutex introduced to fix the threading violation now makes the sync write even more dangerous. If the sync write is blocked, but data is received, the main thread then becomes blocked because the async_read cannot start until the blocked sync write completes.

Would like to understand more why this sync write is being used instead of async. I've heard performance several times but tests I've performed show async write being able to send north of 6GB/sec over a websocket. A single thread with a strand per connection would seem more than capable of handling the demand.

dismissing review

[heifner]

Would still like to understand why async_write performs so poorly in this code where it seems perfectly fine in a simple test program. Hopefully a follow on task can be worked to understand.