dnp3 / opendnp3

DNP3 (IEEE-1815) protocol stack. Modern C++ with bindings for .NET and Java.
https://dnp3.github.io
Apache License 2.0
300 stars 231 forks source link

Keep-alive timer was not properly calculated + LinkContext lifetime issue #407

Closed emgre closed 2 years ago

emgre commented 4 years ago

Fix it and added a unit test.

Fix #392.

codecov[bot] commented 4 years ago

Codecov Report

Merging #407 (23668af) into develop (4372862) will increase coverage by 0.11%. The diff coverage is 63.15%.

@@             Coverage Diff             @@
##           develop     #407      +/-   ##
===========================================
+ Coverage    51.29%   51.40%   +0.11%     
===========================================
  Files          423      423              
  Lines        13439    13444       +5     
===========================================
+ Hits          6893     6911      +18     
+ Misses        6546     6533      -13     
Impacted Files Coverage Δ
cpp/lib/src/link/LinkContext.h 100.00% <ø> (ø)
cpp/lib/src/master/MasterContext.h 100.00% <ø> (ø)
cpp/lib/src/master/MasterStack.h 100.00% <ø> (ø)
cpp/lib/src/master/MasterSessionStack.cpp 27.18% <18.75%> (ø)
cpp/lib/src/master/MasterStack.cpp 19.35% <25.00%> (ø)
cpp/lib/src/link/LinkContext.cpp 91.39% <100.00%> (+1.23%) :arrow_up:
cpp/lib/src/link/LinkLayer.cpp 100.00% <100.00%> (ø)
cpp/lib/src/master/MasterContext.cpp 84.16% <100.00%> (+0.12%) :arrow_up:
cpp/lib/src/transport/TransportLayer.cpp 93.75% <0.00%> (-1.57%) :arrow_down:
cpp/lib/src/link/LinkFrame.cpp 95.06% <0.00%> (-1.24%) :arrow_down:
... and 4 more

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

emgre commented 4 years ago

Since I had to play in LinkContext, I made a fix for #396. This would replace #395. Instead of owning the shared_ptr in the callbacks and waiting in the destructor for them to release, the callbacks instead own a weak_ptr and checks that it's still valid (the shared_ptr automatically gets destroyed in the LinkContext default destructor).

emgre commented 2 years ago

As noted by Nicholas Lee, even with the original fix, the keep-alive timers were internally expiring early, just to restart a very short timer afterwards. This was caused by how we used to drive the keep-alive timer. It was not restarted on every link activity, we were just waiting for the old one to expire and then we were re-calculating a new timer based on the recorded last link activity. I made the change to simply restart the timer on every link activity, I think it's cleaner.

As for the LinkContext lifetime issue, I ended up superseding #435 by simply having the LinkContext be shared_from_this. Shutdown is properly done in the OnLowerLayerDown and checked in the callbacks. I also found that MContext could possibly have a similar issue (if a shutdown occurs while waiting for a response), so I applied the same fix. Shutdown is also properly checked.