Feature/reentrancy #111

art-gor · 2021-02-11T10:23:48Z

This update resolves the following:

No more throws from boost calls;
Async operations are not reentrant (callback passed in arguments is guaranteed not to be called from withing the function);
Yamux redesigned, resolved window issues, data losses, delays due to window management;

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

refactoring: replace deprecated sha256 function by hasher Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

…alidation)

…plaintext

igor-egorov · 2021-03-11T08:53:06Z

Address sanitizer reports a heap-use-after-free error when launching rendezvous_chat at the last commit 5545319. This started to happen only within that pr.

Please check the log attached. Please fix the memory corruption.

asan-3.txt
(the log was collected on Ubuntu 18.04, clang-11.1.0)

…callbacks

igor-egorov

The first phase of review done.

igor-egorov

Thanks!

include/libp2p/basic/reader.hpp

igor-egorov · 2021-03-22T07:01:21Z

include/libp2p/basic/reader.hpp

+     *
+     * @note if (!ec) then this function does nothing
+     */
+    virtual void deferReadCallback(outcome::result<size_t> res,


Callback aleady has an argument of type outcome::result<size_t> - was it expected the first argument to be an error_code like in deferWriteCallback?

If 1 is true, then can we have a single method named something like deferIoCallback?

fixed

trying to avoid diamond hierarchy

igor-egorov · 2021-03-22T07:02:21Z

include/libp2p/basic/writer.hpp

+     *
+     * @note if (!ec) then this function does nothing
+     */
+    virtual void deferWriteCallback(std::error_code ec, WriteCallbackFunc cb) = 0;


Why do we have to pass ec into that method? Can we just do the check outside of that method implementation if needed?

yes we can, but the fn will do nothing anyways

include/libp2p/basic/reader.hpp

src/basic/varint_reader.cpp

igor-egorov · 2021-03-22T07:50:10Z

src/muxer/yamux/yamuxed_connection.cpp

@@ -131,438 +147,535 @@ namespace libp2p::connection {

  void YamuxedConnection::read(gsl::span<uint8_t> out, size_t bytes,
                               ReadCallbackFunc cb) {
-    connection_->read(out, bytes, std::move(cb));
+    log()->critical("YamuxedConnection::read : invalid direct call");
+    deferReadCallback(YamuxError::FORBIDDEN_CALL, std::move(cb));


As we discussed offline - this could be the reason the multiselect protocol gets broken.

It appears that it's not where it breaks, unfortunately

igor-egorov · 2021-03-22T07:53:42Z

src/muxer/yamux/yamuxed_connection.cpp

+  void YamuxedConnection::writeStreamData(uint32_t stream_id,
+                                          gsl::span<const uint8_t> data,
+                                          bool some) {
+    // TODO(artem): reform in buffers (shared + vector writes)


Please fix or create an issue with a clear description. Unfortunately, it does not prescriptive enough of what to do.

include/libp2p/protocol_muxer/multiselect/connection_state.hpp

include/libp2p/transport/tcp/tcp_connection.hpp

src/protocol/echo/server_echo_session.cpp

include/libp2p/basic/reader.hpp

xDimon · 2021-03-16T08:09:41Z

src/basic/read_buffer.cpp

+
+      capacity_remains_ = alloc_granularity_;
+    } else {
+      fragments_.emplace_back();


Is it ok if new fragment will be added only for oversized data?

yes, some heuristic here to minimize copying

xDimon · 2021-03-16T08:12:49Z

src/basic/read_buffer.cpp

+
+      capacity_remains_ -= sz;
+    } else if (capacity_remains_ > 0) {
+      auto &vec = fragments_.back();


Is it ok if parts of data which size less remaining capacity will inflate only one fragment?

yes, only last fragmen's capacity counts

xDimon · 2021-03-16T08:30:27Z

src/basic/read_buffer.cpp

+    total_size_ = 0;
+    first_byte_offset_ = 0;
+    capacity_remains_ = 0;
+    std::deque<Fragment>{}.swap(fragments_);


What is advantages to use swap instead clear?

deallocation

but it will be deallocated anyway in dtor at out of scope

src/basic/read_buffer.cpp

test/libp2p/muxer/muxers_and_streams_test.cpp

xDimon · 2021-03-23T08:15:59Z

test/libp2p/protocol/echo_test.cpp

@@ -66,16 +71,16 @@ TEST(EchoTest, Server) {
 * @when client writes string "hello" to the Stream
 * @then client reads back the same string
 */
-TEST(EchoTest, Client) {
+TEST(EchoTest, DISABLED_Client) {


Comment, please, why is it disabled and issue (number or link) to enable it back.
Or remove if it really needless anymore

this test expects reentrant behavior from mocks and other objects, needs to be rewritten as soon as new testing tools for async appears (net PR)

src/muxer/yamux/yamuxed_connection.cpp

* multiselect revised, WIP * multiselect: simple outbound stream negotiate * multiselect numerous fixes * multiselect: instances and reuse * multiselect: fixes * multiselect: removed old implementation * multiselect: interop with go impl fixes * multiselect: bugfixes * multiselect: ProtocolMuxer interface abstracts simple outbound stream negotiation * multiselect: cleanups and logging * trigger CI * temporarily disabled tests that required synchronous reaction of multiselect * just removed unused lines * reverted back ci.yml

art-gor and others added 30 commits September 30, 2020 12:40

gossip confirmance fixes #1

59c1169

gossip: bugfixes

7ad5334

gossip: more fixes

f967bb2

gossip: build related minor changes

6f28421

gossip: one more fix

2759efc

gossip: flush hello message asap

7193d38

Merge branch 'master' into feature/gossip-fixes

f62ef9b

experiment with noise protocol

384c99d

made injector creation fns inline to prevent multiple definitions

dace7af

Merge branch 'master' into feature/gossip-fixes

a2187fb

gossip restructured, interop issues fixed

8b7f8be

Noise::write now reports the correct amount of bytes

7268449

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

refactoring: move prev implementation of Kademlia to other namespace

c4394e1

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

feature: kademlia dependency injection

d81650a

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

feature: improve random generator

03f2000

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

fix: getting listened multiaddress in tcp listener

4171f0c

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

fix: reduce logging in secio

6e944fc

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

draft: new implementation of kademlia

c87528c

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

draft: processing with kademlia

1b75330

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

draft: continue processing with kademlia

753fa33

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

draft: continue

82ecf6c

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

draft: kademlia

ce16d8f

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

feature: validation

e612cb3

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

fix: DSA

e58bcf4

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

feature: CIDv1 encoding

daa8833

refactoring: replace deprecated sha256 function by hasher Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

fix: multiaddress operations

920c699

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

feature: make PeerId comparable to using in std::set

f5c6f02

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

refactoring: addr repo

1188e84

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

fix: Go-implementation compatibility

68d6ee6

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

feature: rendezvous chat as example of Kademlia using

a3b6ce9

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>

art-gor added 8 commits February 25, 2021 19:21

fixes regarding std::move of r/w callbacks (against possible ptrs inv…

384a61e

…alidation)

test cases with jumbo messages transfer added for yamux on noise/tls/…

0481fed

…plaintext

bugfixes related to yamux window sizes, overflow, and acknowledgements

727247e

echo protocol and examples support very large msgs

9bd776c

changes in tests

8e92c98

all muxers acceptance test recovered

a41cdc0

all muxers acceptance test: fixed issue with mock lifetime

d74c5f2

CI fixes

5545319

art-gor added 3 commits March 11, 2021 19:25

yamux refactorings helped to avoid memory issues caused by reentrant …

fb3b7da

…callbacks

build fix for mac

f124b75

Merge branch 'master' into feature/reentrancy

5841dc0

art-gor marked this pull request as ready for review March 12, 2021 06:08

art-gor requested review from igor-egorov and xDimon March 12, 2021 06:08

another yamux fix to avoid memory issues caused by reentrant callbacks

9094192

igor-egorov mentioned this pull request Mar 16, 2021

Multiselect integration test #118

Open

igor-egorov reviewed Mar 17, 2021

View reviewed changes

igor-egorov approved these changes Mar 22, 2021

View reviewed changes

xDimon reviewed Mar 23, 2021

View reviewed changes

art-gor added 4 commits March 23, 2021 15:48

gossip: hotfix related to unbanning peers

e0ba211

minor fixes reflecting review feedback

ae85786

Merge branch 'master' into feature/reentrancy

7559d24

temp loggers workaround

4a4d215

igor-egorov requested a review from xDimon March 24, 2021 14:42

xDimon approved these changes Apr 1, 2021

View reviewed changes

art-gor added 2 commits April 2, 2021 13:25

Merge branch 'master' into feature/reentrancy

7dfdaba

art-gor merged commit 8d1b7ef into master Apr 2, 2021

art-gor deleted the feature/reentrancy branch April 2, 2021 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/reentrancy #111

Feature/reentrancy #111

art-gor commented Feb 11, 2021

igor-egorov commented Mar 11, 2021

igor-egorov left a comment

igor-egorov left a comment

igor-egorov Mar 22, 2021

art-gor Mar 23, 2021

igor-egorov Mar 22, 2021

art-gor Mar 23, 2021

igor-egorov Mar 22, 2021

art-gor Mar 23, 2021

igor-egorov Mar 22, 2021

xDimon Mar 16, 2021

art-gor Mar 23, 2021

xDimon Mar 16, 2021

art-gor Mar 23, 2021

xDimon Mar 16, 2021

art-gor Mar 23, 2021

xDimon Mar 23, 2021

xDimon Mar 23, 2021

art-gor Mar 23, 2021 •

edited

Feature/reentrancy #111

Feature/reentrancy #111

Conversation

art-gor commented Feb 11, 2021

igor-egorov commented Mar 11, 2021

igor-egorov left a comment

Choose a reason for hiding this comment

igor-egorov 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

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

art-gor Mar 23, 2021 • edited

Choose a reason for hiding this comment

art-gor Mar 23, 2021 •

edited