Asynchronous processing support #50

enginespot · 2019-05-18T11:03:55Z

Add Asynchronous processing support #31

tomtau

As mentioned in the related issue discussion, this looks OK -- I'd wait for comments from @ebuchman @melekes @liamsi.

I'd only request two changes:

integrate / add tests -- the previous code worked over "AbciStream" and the point was that this could abstract over the underlying connection -- a mock connection in tests, TCP or UDS in application code.
remove the Copy trait requirement on ABCI app.
That will have some implications later in the code that could be resolved in different ways:

bring the Arc<Mutex<...>> back for the moment (if the latter incurs big changes)
shuffle the execution pipeline to be something like this:

(thread pool for async reading/parsing of ABCI requests from different connections)
=>
(a dedicated single thread owning ABCI app for generating ABCI responses)
=>
(thread pool for async writing of ABCI responses to corresponding connections)

tomtau · 2019-05-21T01:24:16Z

src/lib.rs

@@ -101,7 +105,7 @@ pub trait Application {
 /// Setup the app and start the server using localhost and default tendermint port 26658
 pub fn run_local<A>(app: A)
 where
-    A: Application + 'static + Send + Sync,
+    A: Application + 'static + Send + Sync + Copy,


this will potentially break client code -- not all ABCI applications will implement "Copy", e.g. one of fields could be a DB connection.

Hi, @tomtau I have removed "Copy" from the latest version, please check.

tomtau · 2019-05-21T02:05:09Z

src/server.rs


-    // Wrap the app atomically and clone for each connection.
-    let app = Arc::new(Mutex::new(app));


Not sure if this can be removed (and Copy can't really be required, as mentioned in https://github.com/tendermint/rust-abci/pull/50/files#diff-b4aea3e418ccdb71239b96952d9cddb6R108 ) . When I mentioned in #31 (comment) that there may not be a reason for mutex, I meant the following possibilities:

if the request reading / parsing is separate from response generation, the response generation could potentially be in one system thread, so no need for app synchronisation / locking... the app could then say be wrapped say in Rc<Cell<...>> or owned within that "response generating" thread's scope

if the Application trait is changed, such that it doesn't require a mutable ref in Mempool/Info connection-related operations (CheckTx, Query...), there could be a RwLock instead of Mutex... this could be good, but could also lead to some problems, e.g. a writer starvation.

if the Application trait is split into some sort of "Query" and "Update" parts and it's somehow possible to distinguish among those 3 ABCI connections, it may be possible to use them without dynamic wrapping -- Mempool and Info connections would get "&ApplicationQuery", Consensus connection would get "&mut ApplicationUpdate".

Perhaps the first possibility can be easily applied here, but if not, I'd leave it for now and open a separate issue for this, because it requires a significant re-architecting of rust-abci.
@davebryson any thoughts?

tomtau

Looks ok for start. Just one missing bit is basic tests -- before it operated over this stream: https://github.com/tendermint/rust-abci/blob/master/src/stream.rs#L12 So perhaps that could be somehow modified to use Tokio stuff?

tomtau · 2019-05-24T07:14:41Z

src/lib.rs

@@ -10,6 +10,7 @@
 //! the Trait.  The app doesn't do any actual processing on a transaction.
 //!
 //! ```rust,no_run
+//! #[derive(Clone, Copy)]


shouldn't be needed

Resolved in the version
f3f4653

tomtau · 2019-05-24T07:15:36Z

src/codec/abci.rs

+        buf.put(&varint);
+        buf.reserve(1 + msg_len as usize);
+        msg.write_to_writer(&mut buf.writer()).unwrap();
+        println!("Encode response! {:?}", &buf[..]);


merge with the latest master and use e.g. info! logger call instead

Resolved in the version
eecdf56

enginespot · 2019-05-24T07:34:10Z

Looks ok for start. Just one missing bit is basic tests -- before it operated over this stream: https://github.com/tendermint/rust-abci/blob/master/src/stream.rs#L12 So perhaps that could be somehow modified to use Tokio stuff?

hmm... in my opinion, ABCI protocol is based on TcpStream, we do not need to mock TcpStream, so I think maybe it will better to remove the stream.rs file, but we need to add some test for ABCI codec, it only has a relationship with data codec but not stream

if we do need to add some mock TcpStream, maybe it will be better to add some test to server.rs, since that it has some relationship with the data stream

tomtau · 2019-05-24T09:24:47Z

hmm... in my opinion, ABCI protocol is based on TcpStream, we do not need to mock TcpStream, so I think maybe it will better to remove the stream.rs file, but we need to add some test for ABCI codec, it only has a relationship with data codec but not stream

Well, ABCI can run over TCP or Unix Domain sockets -- see #47 so ideally encoding/decoding is abstracted from that

2.refactoring abci.rs and add error handling

tomtau

@enginespot the latest changes look good -- I think removing ABCIStream make sense; the use of ABCICode should have the similar benefits (for testing and later extending it to run over Unix Domain Sockets instead of TCP).
The remaining change is that it seems this PR accidentally reverted the changes from this PR: https://github.com/tendermint/rust-abci/pull/48/files which would break the compatibility with the latest Tendermint version. so either reset changes in src/messages/* or you can run make update-proto

tomtau · 2019-05-27T01:59:07Z

src/messages/abci.rs

@@ -7550,7 +7550,7 @@ impl ::protobuf::reflect::ProtobufValue for ResponseCommit {
 #[derive(PartialEq,Clone,Default)]
 pub struct ConsensusParams {
    // message fields
-    pub block: ::protobuf::SingularPtrField<BlockParams>,
+    pub block_size: ::protobuf::SingularPtrField<BlockSizeParams>,


@enginespot seems this overrides the latest changes from protobuf definitions -- Tendermint 0.2* had block_size, but 0.3* has block -- see https://github.com/tendermint/rust-abci/pull/48/files

tomtau · 2019-05-27T01:59:48Z

src/messages/merkle.rs

@@ -443,8 +443,8 @@ static file_descriptor_proto_data: &'static [u8] = b"\
    rkle\">\n\x07ProofOp\x12\x10\n\x04type\x18\x01\x20\x01(\tB\x02\x18\0\x12\
    \x0f\n\x03key\x18\x02\x20\x01(\x0cB\x02\x18\0\x12\x10\n\x04data\x18\x03\
    \x20\x01(\x0cB\x02\x18\0\")\n\x05Proof\x12\x20\n\x03ops\x18\x01\x20\x03(\
-    \x0b2\x0f.merkle.ProofOpB\x02\x18\0B\x14\xa8\xe2\x1e\x01\xe0\xe2\x1e\x01\
-    \xd0\xe2\x1e\x01\xc8\xe2\x1e\x01\xf8\xe1\x1e\x01b\x06proto3\
+    \x0b2\x0f.merkle.ProofOpB\x02\x18\0B\x14\xc8\xe2\x1e\x01\xf8\xe1\x1e\x01\


again this probably reverts the latest tendermint protobuf changes https://github.com/tendermint/rust-abci/pull/48/files

tomtau · 2019-05-27T01:59:59Z

src/messages/types.rs

@@ -429,8 +429,8 @@ static file_descriptor_proto_data: &'static [u8] = b"\
    n\",\n\x06KVPair\x12\x0f\n\x03key\x18\x01\x20\x01(\x0cB\x02\x18\0\x12\
    \x11\n\x05value\x18\x02\x20\x01(\x0cB\x02\x18\0\".\n\x08KI64Pair\x12\x0f\
    \n\x03key\x18\x01\x20\x01(\x0cB\x02\x18\0\x12\x11\n\x05value\x18\x02\x20\
-    \x01(\x03B\x02\x18\0B\x1c\xd0\xe2\x1e\x01\xc0\xe3\x1e\x01\xe0\xe2\x1e\
-    \x01\xa8\xe2\x1e\x01\xf8\xe1\x1e\x01\xb8\xe2\x1e\x01\xc8\xe2\x1e\x01b\
+    \x01(\x03B\x02\x18\0B\x1c\xa8\xe2\x1e\x01\xc0\xe3\x1e\x01\xd0\xe2\x1e\


as mentioned before https://github.com/tendermint/rust-abci/pull/48/files

liamsi · 2019-05-28T12:50:20Z

@tomtau you should have merge perms now (?). Can you squash-merge this to master and then create a PR that merges master to develop? All sub-sequent PRs should be against develop instead of master (only PRs that are there to trigger then next release should be from develop to master).

- Changed to try_init, if init twice program panics - Added into example apps the init of the logger - Bump package Version Signed-off-by: Marko Baricevic <[email protected]>

… enginespot-master

enginespot added 2 commits May 18, 2019 18:58

Asynchronous processing support

427c5a5

Asynchronous processing support

cd9566a

enginespot marked this pull request as ready for review May 18, 2019 15:02

enginespot added 4 commits May 18, 2019 23:29

Asynchronous processing support

307208c

Asynchronous processing support

a79185e

Asynchronous processing support

5473f32

Asynchronous processing support

5035d39

tomtau suggested changes May 21, 2019

View reviewed changes

tomtau mentioned this pull request May 21, 2019

Asynchronous processing #31

Closed

enginespot added 2 commits May 23, 2019 16:25

Asynchronous processing support

7ddf403

Asynchronous processing support

9899a32

tomtau suggested changes May 24, 2019

View reviewed changes

tomtau reviewed May 24, 2019

View reviewed changes

Asynchronous processing support

f3f4653

tomtau reviewed May 24, 2019

View reviewed changes

Asynchronous processing support

eecdf56

enginespot added 2 commits May 27, 2019 04:05

1.remove server.rs

241a27d

2.refactoring abci.rs and add error handling

move test case from stream.rs to codec.rs

0489c64

tomtau suggested changes May 27, 2019

View reviewed changes

merge abci.rs/merkle.rs/types.rs with master branch

baf66ee

tomtau approved these changes May 27, 2019

View reviewed changes

tac0turtle and others added 3 commits May 29, 2019 09:43

Init Logger fix (tendermint#56)

d8bea64

- Changed to try_init, if init twice program panics - Added into example apps the init of the logger - Bump package Version Signed-off-by: Marko Baricevic <[email protected]>

Merge branch 'master' of https://github.com/enginespot/rust-abci into…

d915016

… enginespot-master

Merge branch 'enginespot-master'

9b4cfca

tomtau merged commit 4d12a12 into tendermint:master May 29, 2019

This was referenced May 29, 2019

Rearchitecting Rust ABCI #61

Closed

Sync up develop with master #62

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asynchronous processing support #50

Asynchronous processing support #50

enginespot commented May 18, 2019

tomtau left a comment •

edited

Loading

tomtau May 21, 2019

enginespot May 24, 2019

tomtau May 21, 2019

tomtau left a comment

tomtau May 24, 2019

enginespot May 24, 2019

tomtau May 24, 2019

enginespot May 24, 2019 •

edited

Loading

enginespot commented May 24, 2019

tomtau commented May 24, 2019

tomtau left a comment

tomtau May 27, 2019

tomtau May 27, 2019

tomtau May 27, 2019

liamsi commented May 28, 2019


		// Wrap the app atomically and clone for each connection.
		let app = Arc::new(Mutex::new(app));

Asynchronous processing support #50

Asynchronous processing support #50

Conversation

enginespot commented May 18, 2019

tomtau left a comment • 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

tomtau left a 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

enginespot May 24, 2019 • edited Loading

Choose a reason for hiding this comment

enginespot commented May 24, 2019

tomtau commented May 24, 2019

tomtau left a 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

liamsi commented May 28, 2019

tomtau left a comment •

edited

Loading

enginespot May 24, 2019 •

edited

Loading