Aleth waits for 2 seconds after sending disconnect packet by twinstar26 · Pull Request #5707 · ethereum/aleth

twinstar26 · 2019-07-30T13:36:02Z

codecov-io · 2019-07-30T14:18:40Z

Codecov Report

Merging #5707 into master will increase coverage by 0.07%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master    #5707      +/-   ##
==========================================
+ Coverage   63.06%   63.14%   +0.07%     
==========================================
  Files         353      353              
  Lines       30196    30193       -3     
  Branches     3386     3386              
==========================================
+ Hits        19044    19064      +20     
+ Misses       9918     9896      -22     
+ Partials     1234     1233       -1

halfalicious · 2019-07-30T15:48:24Z

@twinstar26 Is this ready for review? If so, can you please request a review from myself / gumb0 / chfast?

halfalicious · 2019-07-31T03:20:13Z

CHANGELOG.md

 - Added: [#5634](https://github.com/ethereum/aleth/pull/5634) Bootnodes for Rinkeby and Goerli.
 - Added: [#5640](https://github.com/ethereum/aleth/pull/5640) Istanbul support: EIP-1702 Generalized Account Versioning Scheme.
 - Added: [#5690](https://github.com/ethereum/aleth/issues/5690) Istanbul support: EIP-2028 transaction data gas cost reduction.
+- Added: [#5650](https://github.com/ethereum/aleth/issues/5650) Aleth should wait for 2 seconds to close socket after sending disconnect to peer.


Changelog should describe the behavior with your fix applied (e.g. "Aleth now waits for 2 seconds after sending disconnect to peer before closing socket")

halfalicious · 2019-07-31T03:31:33Z

libp2p/Session.cpp

        RLPStream s;
        prep(s, DisconnectPacket, 1) << (int)_reason;
        sealAndSend(s);
+        auto disconnectTimer = m_server->createTimer();


Session::disconnect will still close the socket immediately (Session::drop closes the socket and you're calling it in both your timer handler and outside the handler if it hasn't already been called i.e. !m_dropped). Since what we want to do is delay the socket closure for 2 seconds, you should move the code which closes the socket out of Session::drop and into the timer handler and also not call drop inside of the timer handler.

@halfalicious
Thanks for the reviews!
Okay so I have to copy required logic of drop into disconnect and never call drop from inside of disconnect and its handler. Am I right??

You should move the socket closure code out of drop and into the handler. Don’t call drop inside of the handler but still call it in your else statement in disconnect (since we want to prevent further session operations after we have sent the disconnect, you can see that we check for m_drop in various places in session and return early)

@halfalicious
So pushed changes according to your suggested changes. But was curious what will happen to code thats calling drop() (apart from disconnect) separately??

This still doesn't look right:

Changing drop() now breaks all the cases where it is called from other methods of Session. I guess those are reasonable cases where we wanted to just disconnect immediately without sending Disconnect packet (with current changes we won't disconnect)

Calling disconnect now doesn't ever set m_dropped to true. @halfalicious Maybe you meant calling current form of drop from inside if (!m_dropped) clause?

@gumb0 You're right we need to also call drop() inside of the !m_dropped clause.

I don't think this new form of drop breaks anything though...even if drop doesn't close the socket all shared_ptr Session instances will still go out of scope and therefore Session will be garbage collected by Host and the socket will be closed when the dtor is invoked. Note that functions in which we pass new shared_ptr instances (shared_from_this) to handlers check if (m_dropped) and then prematurely terminate before creating new handlers (e.g. doRead). In fact, after taking a closer look at the Session code, I don't think we need to explicitly close the socket at all in any place other than the dtor (unless there's some benefit to closing the socket sooner rather than later that I'm not aware of)? If we just call drop() without closing the socket the Session instance will be destroyed once all queued handlers which contain a shared_ptr instance are executed and the Host garbage collects the Session.

So, to summarize:

I think we should call drop() in both the if (!m_dropped) and else cases...in fact, we can simplify this by removing the "else" case and always calling drop() after the if (!m_dropped) code block since drop returns early if m_dropped is true.

I think we can remove all cases where we close the socket except for in the dtor.

@gumb0 : Thoughts?

I don't think we need to explicitly close the socket at all in any place other than the dtor (unless there's some benefit to closing the socket sooner rather than later that I'm not aware of)?

@halfalicious The benefit I see is that we immediately stop answering any further requests on this connection, not waiting for all handlers to terminate, so maybe it's more efficient...

Also, what if some malicious peer keeps DOSing us with some invalid requests on this connection - it will be kept in the handler queue (because the new data keeps coming) and never disconnected, it seems so.

@halfalicious pointed out to me:

I think that we will still stop reading packets from a peer if we call drop without closing the socket since we check for m_dropped at the beginning of Session::doRead before proceeding with the read:

aleth/libp2p/Session.cpp

Lines 318 to 322 in d8f4498

void Session::doRead()

{

// ignore packets received while waiting to disconnect.

if (m_dropped)

return;

I think I agree with this reasoning.

One corner case to check: spec requires that we disconnet immediately upon receiving Disconnect packet ourselves. My experiment confirms that this looks to be working correctly with current changes in this PR:

TRACE 08-08 11:54:24 p2p p2pcap Disconnect from (##a8768590…@51.141.78.53:30303) DEBUG 08-08 11:54:24 p2p p2pcap Disconnect (reason: Peer had too many connections.) from (##a8768590…@51.141.78.53:30303) DEBUG 08-08 11:54:24 p2p net Closing peer session with (##a8768590…@51.141.78.53:30303)

One corner case to check: spec requires that we disconnet immediately upon receiving Disconnect packet ourselves. My experiment confirms that this looks to be working correctly with current changes in this PR:

TRACE 08-08 11:54:24 p2p p2pcap Disconnect from (##a8768590…@51.141.78.53:30303) DEBUG 08-08 11:54:24 p2p p2pcap Disconnect (reason: Peer had too many connections.) from (##a8768590…@51.141.78.53:30303) DEBUG 08-08 11:54:24 p2p net Closing peer session with (##a8768590…@51.141.78.53:30303)

@gumb0 Confirmed by looking at the code - when we receive a disconnect packet we call drop immediately in Session::interpretP2pPacket:

aleth/libp2p/Session.cpp

Lines 142 to 154 in 78355a2

case DisconnectPacket:

{

string reason = "Unspecified";

auto r = (DisconnectReason)_r[0].toInt<int>();

if (!_r[0].isInt())

drop(BadProtocol);

else

{

reason = reasonOf(r);

LOG(m_capLogger) << "Disconnect (reason: " << reason << ") from";

drop(DisconnectRequested);

}

break;

gumb0 · 2019-08-08T09:33:06Z

libp2p/Session.cpp

+    RLPStream s;
+    prep(s, DisconnectPacket, 1) << (int)_reason;
+    sealAndSend(s);
+    auto disconnectTimer = m_server->createTimer();


I think this timer will be destroyed right after this function ends, and outstanding wait operation will be cancelled. (Probably should be made a member?)

@gumb0 Agreed, in my prototype I had captured the timer in the handler but making it a member works as well (and also enables us to make it unique_ptr rather than shared_ptr as I see you've done) 😄

gumb0 · 2019-08-08T09:49:11Z

I moved the calls to timer's member inside createTimer() function to hide these lower-level details from Host's users and for better symmetry between postWork and createTimer

…sing socket

gumb0 · 2019-08-08T10:07:59Z

Rebased.

gumb0 · 2019-08-08T10:13:58Z

libp2p/Session.cpp

+    m_disconnectTimer = m_server->createTimer(
+        std::chrono::seconds(2), [self, this, _reason](boost::system::error_code) {
+            bi::tcp::socket& socket = m_socket->ref();
+            if (socket.is_open())


One thing I still don't like here is that we have the same code (closing the socket) here in the timer handler and then in destructor. And destructor is executed anyway, so perhaps we could avoid this duplication, maybe just posting empty handler? @halfalicious what do you think?

@gumb0 Yes I agree with this, in fact I suggested this earlier in this PR 😄

halfalicious · 2019-08-13T04:35:56Z

Confirmed that we wait for 2 seconds now:

TRACE 08-12 21:33:11 p2p  p2pcap Disconnecting (our reason: Peer had no use for this node.) from (##03957ecc…@51.75.127.66:30303)
TRACE 08-12 21:33:11 p2p  net    Disconnect to (##03957ecc…@51.75.127.66:30303)
...
DEBUG 08-12 21:33:13 p2p  net    Closing peer session with (##03957ecc…@51.75.127.66:30303)

halfalicious · 2019-08-13T05:08:17Z

@gumb0 I think this is ready to be merged, please give it a quick look when you get a chance

gumb0 · 2019-08-15T15:33:58Z

libp2p/Host.h


    std::shared_ptr<CapabilityHostFace> capabilityHost() const { return m_capabilityHost; }

+    /// Execute work on the network thread after an @_expiryDelay delay.


That was doxygen syntax to refer to the parameter inside description
(http://www.doxygen.nl/manual/commands.html#cmda)

Suggested change

/// Execute work on the network thread after an @_expiryDelay delay.

/// Execute work on the network thread after an @a _expiryDelay delay.

@gumb0 That's what I suspected but I saw different syntax used in other parts of the project e.g.:

aleth/libethereum/Account.h

Lines 150 to 152 in 78355a2

/// @returns account's original storage value corresponding to the @_key

/// not taking into account overlayed modifications

u256 originalStorageValue(u256 const& _key, OverlayDB const& _db) const;

Closing the socket in the handler is unnecessary since it will be closed when the dtor executes.

halfalicious requested review from chfast, gumb0 and halfalicious and removed request for chfast and gumb0 July 30, 2019 16:21

halfalicious reviewed Jul 31, 2019

View reviewed changes

gumb0 mentioned this pull request Jul 31, 2019

[WIP] Aleth now waits for 2 secs after sending disconnect packet #5650 #5658

Closed

twinstar26 force-pushed the master branch from 1be6ed1 to da6d587 Compare July 31, 2019 17:38

gumb0 reviewed Aug 8, 2019

View reviewed changes

gumb0 added the in progress label Aug 8, 2019

twinstar26 and others added 3 commits August 8, 2019 12:06

Aleth waits for 2 seconds after sending disconnect to peer before clo…

ad75763

…sing socket

Correctly call drop() from disconnect()

467c8c6

Call expires_after and async_wait inside Host::createTimer

bf7b9ae

gumb0 force-pushed the master branch from 2b94b15 to bf7b9ae Compare August 8, 2019 10:07

gumb0 reviewed Aug 8, 2019

View reviewed changes

halfalicious removed the in progress label Aug 13, 2019

gumb0 reviewed Aug 15, 2019

View reviewed changes

gumb0 approved these changes Aug 15, 2019

View reviewed changes

Don't close socket in disconnect timer handler

d7bd87c

Closing the socket in the handler is unnecessary since it will be closed when the dtor executes.

halfalicious force-pushed the master branch from de74c8c to d7bd87c Compare August 16, 2019 03:04

halfalicious merged commit bb4721f into ethereum:master Aug 16, 2019

halfalicious mentioned this pull request Nov 19, 2019

Aleth 1.7.0 crash on Ubuntu 18.04 LTS #5832

Closed

	void Session::doRead()
	{
	// ignore packets received while waiting to disconnect.
	if (m_dropped)
	return;

	case DisconnectPacket:
	{
	string reason = "Unspecified";
	auto r = (DisconnectReason)_r[0].toInt<int>();
	if (!_r[0].isInt())
	drop(BadProtocol);
	else
	{
	reason = reasonOf(r);
	LOG(m_capLogger) << "Disconnect (reason: " << reason << ") from";
	drop(DisconnectRequested);
	}
	break;


		std::shared_ptr<CapabilityHostFace> capabilityHost() const { return m_capabilityHost; }

		/// Execute work on the network thread after an @_expiryDelay delay.

	/// @returns account's original storage value corresponding to the @_key
	/// not taking into account overlayed modifications
	u256 originalStorageValue(u256 const& _key, OverlayDB const& _db) const;

Conversation

twinstar26 commented Jul 30, 2019

Uh oh!

codecov-io commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

halfalicious commented Jul 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumb0 Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumb0 commented Aug 8, 2019

Uh oh!

gumb0 commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious commented Aug 13, 2019

Uh oh!

halfalicious commented Aug 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jul 30, 2019 •

edited

Loading

halfalicious Jul 31, 2019 •

edited

Loading

gumb0 Aug 2, 2019 •

edited

Loading

halfalicious Aug 13, 2019 •

edited

Loading

halfalicious Aug 13, 2019 •

edited

Loading