Sync node branch #425

aboimpinto · 2018-09-20T10:29:38Z

several traces added in order to help finding the issue of disconnection of the peer.

IContainer dependency in MessageHandlerProxy removed and Strategy pattern is in place.

…terface but it's not anymore, threfore should not carry the prefix "I" that identify the interfaces.

…into development

Black listed peers try to connect test

…ge of the method RandomInt. * Small clean up on the UtCrypto

more tests will be added soon!

…into development

…ymore.

…into development

* Fix an inverted if logic. The code only enter in the While loop if the cancellation was requested and should be the other way around.

osmirnov

What is the goal for these changes? I am a strong beliver MVP should be based on the current code version instead of introduction a new project and making code to fit MVP. I suggest not to merge it into development but split it separate PRs that helps to run MVP with NeoSharp.Application.

osmirnov · 2018-09-20T13:40:25Z

src/NeoSharp.Core/DI/Modules/NetworkModule.cs

-            containerBuilder.Register(typeof(IMessageHandler<>), messageHandlerTypes);
-            containerBuilder.RegisterInstanceCreator<IMessageHandler<Message>>(c =>
-                new MessageHandlerProxy(c, messageHandlerTypes, c.Resolve<ILogger<MessageHandlerProxy>>()));
+            containerBuilder.RegisterCollection(typeof(IMessageHandler<>), messageHandlerTypes);


What's the point in double registration?

For sure is not need... maybe was because of the MessageHandlerProxy registration that I left there.

this class will have his own interface and don't need to share the IMessageHandler or even the new IMessageHandler.

osmirnov · 2018-09-20T13:41:26Z

src/NeoSharp.Core/DI/Modules/NetworkModule.cs

+            containerBuilder.RegisterCollection(typeof(IMessageHandler<>), messageHandlerTypes);
+            containerBuilder.RegisterCollection(typeof(IMessageHandler), new[]
+            {
+                typeof(VersionMessageHandler),


Now we need to remember to add it here. In previous version all handlers were discoverable based on message type.

That can be done too ... and add to this list
This solution was to make it work with the direct injection of the list. Now we can refactor this in order to get this list any way we want where the last discovery feature can be implemented.

osmirnov · 2018-09-20T13:43:05Z

src/NeoSharp.Core/Messaging/IMessageHandler[TMessage].cs

    {
        Task Handle(TMessage message, IPeer sender);
    }
+
+    public interface IMessageHandler


It is not type safe version.

What you mean "not type safe"? we don't need the message type.

I mean the handler can accept a message with any Message type. We do need message type because the first thing we do inside Handle method is casting to the right type

for sure we can find a solution for that .. using the IContainer inside this class was not right where there is patterns that solve this without using containers.

Would be nice to discuss a better solution about this where we don't use the IContainer, where er don't create the class already with the constructor and where we don't use know patterns like Strategy.

I will create a issue to remember this when we refactor this.

I am not a fan to pass IContainer as a dependency and it is the very last resort for me but, as you can see, we either do it and benefit from strongly typed code or we don't and have weakly typed code. My preference is the former. That being said, I would recommend reverting this change (untyped IMessageHandler interface) until we have a full identical solution. I prefer not to replace the working code with partial one backed by the issue.

Just to tell you the working code is not working .. that why was the change!!!

I'm glad that you aren't a fan of patterns ... then we can discuss that.
This PR is to fix something that wasn't working and wasn't against the code guidelines promoted by @gubanotorious indicating that we need to use known patterns as much we can.

osmirnov · 2018-09-20T13:44:28Z

src/NeoSharp.Core/Messaging/MessageHandlerProxy.cs

-
-            return handleMessageTask.ContinueWith(t =>
+            var isHandled = false;
+            foreach (var handler in this._messageHandlers)


We cannot iterate through all message handler for every incoming message. It will be, for sure, a bottleneck.

of course not .. will be a linq query similar of what we had that, from all the MessageHadlers will return the only one that satisfy the condition.

it is still iterating even being hidden in LINQ

we were already doing the same ... we where receiving a list and doing that ... this is just a clean way without using Reflection to create the objects.

What we are doing is we ask DI container to create the type safe version (that has Handle method with the first argument of actual Message type) of a message handler per message.

What this change introduces is it ask DI container to create the type unsafe version (that has Handle method with the first argument of generic Message type) of a message handler as singleton (single message handler will handle all compatible messages while the app is running) and, in addition, it searches per message through message handler instances to discover who first will be able to handle the message.

I think Simple Injector allow that we can have Type Safe list injected in the constructor. I could not find a constructive example, just the documentation on their website

https://simpleinjector.readthedocs.io/en/latest/advanced.html?highlight=Collection

I will create a Issue requesting that this could be refactor and how.

issue #427 added

Made a small refactor on the Handle method where the Foreach is removed.

I'm not a fan of the if statement
if (messageHandler == null)
{
this._logger.LogError($"could not find the handler for the message type {message.GetType()}");
return;
}
but I think that for now, until this method is not stable we can live with this. The LINQ query should return Single() because all the possible Messages should have a MessageHandler, if not, the developer didn't made is work well.

src/NeoSharp.Core/Network/Server.cs

osmirnov · 2018-09-20T13:47:35Z

src/NeoSharp.Core/Network/Tcp/TcpPeer.cs

@@ -125,7 +125,8 @@ public bool ChangeProtocol(VersionPayload version)
        public void Disconnect()
        {
            Dispose();
-            _logger.LogInformation("The peer was disconnected");
+            _logger.LogInformation($"The peer {this.EndPoint.Host}:{this.EndPoint.Port} was disconnected");
+            this._stream.Dispose();


The stream has to be disposed already in Dispose method call.

You are right!!! Still trying here... and learning a little bit about this.

the problem is this: after the peer is been disconnected I could not find a way to connected it again ... I try to dispose everything in order to call again Socket.Connect I got always a message saying: "the current socket is connected." (or something like this.

this will be clean up

CityOfZion#426

aboimpinto · 2018-09-20T16:07:22Z

MVP project will be removed. I thought to add it because would be nice to debug the Sync problem that we have and have a MVP / Feasibility project that works would help debug.

…g the dispose.

src/NeoSharp.Application/Controllers/PromptBlockchainController.cs

osmirnov · 2018-09-21T13:57:15Z

src/NeoSharp.Core/Messaging/MessageHandlerProxy.cs

-        {
-            const string handleMethodName = nameof(IMessageHandler<Message>.Handle);
-            const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy | BindingFlags.Instance | BindingFlags.Public;
+            var messageHandler = this._messageHandlers.SingleOrDefault(x => x.CanHandle(message));


As I said before, this change alters the behavior we introduced before (message handlers per message vs singletons).
I am not a fan to pass IContainer as a dependency and it is the very last resort for me but, as you can see, in this case we need to have dynamic routing based on message type and to create message handlers on demand. I suggest to keep it in this was way to avoid possible side effects in message handlers. At present, any message handler can be disposed after a message is handled. My recommendation is reverting these changes (untyped message handlers, message handlers as singletons, iteration trough all message handlers for every message) until we have a full identical solution. I prefer not to replace the working code with partial one backed by the issue.

I would like to remember that this code was not working and no one was understanding due the fact that was not fowling the standards.
Thanks for you opinion. It is important for us to move foward but this code was not fowling the code guidelines defined by @gubanotorious where we should use code patterns. This code was weak, using Reflection with a lot of loops to create the MesssageHandler and MessageInvokers. Now it's just have one loop and you prefer the old code.

I'm sure we can improve this code in the refactor phase. For now the code is working with no bugs.

…oxy has his own interface now.

…rnov

…rnov CityOfZion#427

Increase buffer size (CityOfZion#431)

…nd an abstract class. CityOfZion#427

codecov · 2018-09-24T21:34:00Z

Codecov Report

Merging #425 into development will decrease coverage by 0.59%.
The diff coverage is 17.25%.

@@              Coverage Diff               @@
##           development     #425     +/-   ##
==============================================
- Coverage        54.63%   54.03%   -0.6%     
==============================================
  Files              297      296      -1     
  Lines            12238    12238             
==============================================
- Hits              6686     6613     -73     
- Misses            5552     5625     +73

Impacted Files	Coverage Δ
src/NeoSharp.Core/Models/BlockBase.cs	`0% <ø> (ø)`	⬆️
src/NeoSharp.Core/Models/MinerTransaction.cs	`100% <ø> (ø)`	⬆️
src/NeoSharp.Core/Models/ClaimTransaction.cs	`100% <ø> (ø)`	⬆️
src/NeoSharp.Core/Models/InvocationTransaction.cs	`80.55% <ø> (ø)`	⬆️
src/NeoSharp.Core/Models/Witness.cs	`27.5% <ø> (+3.58%)`	⬆️
src/NeoSharp.Core/Models/IssueTransaction.cs	`100% <ø> (ø)`	⬆️
src/NeoSharp.Core/Models/Transaction.cs	`100% <ø> (ø)`	⬆️
src/NeoSharp.Core/Models/PublishTransaction.cs	`100% <ø> (ø)`	⬆️
src/NeoSharp.Core/Models/ContractTransaction.cs	`100% <ø> (ø)`	⬆️
src/NeoSharp.Core/Models/RegisterTransaction.cs	`100% <ø> (ø)`	⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bb7763...2975a70. Read the comment docs.

aboimpinto · 2018-09-25T23:54:02Z

thank!!!

aboimpinto and others added 30 commits July 10, 2018 02:13

RocksDbRepository reach 42% cover

9c016e6

merge with development

c1cb900

Rename abstract class ICrypto to Crypto. I'm sure this was once an in…

17eccae

…terface but it's not anymore, threfore should not carry the prefix "I" that identify the interfaces.

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

d380115

…into development

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

45915e8

…into development

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

bd9a3ff

…into development

merge with development

318cefc

fix some merge problems

bf48c58

Increase Server tests coverage to 94%.

162316b

Black listed peers try to connect test

merge with development

afd6e97

fix merge issues

304fd44

* Remove protected fields from TestBase and replace with overload usa…

3d82e92

…ge of the method RandomInt. * Small clean up on the UtCrypto

Create UtBlockchain and create first tests.

7c7c325

more tests will be added soon!

fix issues from the merge

93077cc

Adding testing information to the team Contributing page.

2c05de5

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

53c78d3

…into development

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

b1ee9aa

…into development

Merge branch 'development' of https://github.com/aboimpinto/neo-sharp …

ef4a592

…into development

Small CleanUp the test class with properties that aren't been used an…

90e29f1

…ymore.

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

c55c654

…into development

UtBlockChain: InitializeBlockchain_IsGenesisBlock_InitializeComplete

23d4063

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

8931884

…into development

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

4e489e8

…into development

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

97d8d68

…into development

Firsts UtBlockProcessor tests

7ec17e4

Merge branch 'development' of https://github.com/aboimpinto/neo-sharp …

a2af9ca

…into development

merge with development

682f44c

AddBlock tests added

1ee33bb

Merge branch 'development' of https://github.com/CityOfZion/neo-sharp …

ffc1d27

…into development

* UtBlockProcessor: Run test

c250c36

* Fix an inverted if logic. The code only enter in the While loop if the cancellation was requested and should be the other way around.

osmirnov suggested changes Sep 20, 2018

View reviewed changes

Replace Blockchain class dependencies with BlockchainContext.

68fcb2e

CityOfZion#426

aboimpinto and others added 5 commits September 20, 2018 18:08

NeoSharp.SyncMVP removed

ec03f3c

Merge branch 'development' into SyncNodeBranch

f2a9d85

Remove NetworkStream.Dispose because it's already called called durin…

14dbd37

…g the dispose.

merge with development

bab8985

merge with development

cac4582

osmirnov reviewed Sep 20, 2018

View reviewed changes

src/NeoSharp.Application/Controllers/PromptBlockchainController.cs Show resolved Hide resolved

aboimpinto added 2 commits September 21, 2018 01:38

refactor the Handle method to remove the foreach and nested if statement

0c1d390

add appsetting.neo-privnet.json

f62fdfb

osmirnov reviewed Sep 21, 2018

View reviewed changes

aboimpinto and others added 8 commits September 22, 2018 01:40

Missing cleanup when <object>OperationManager classes were introduced.

41167fb

Cleanup the registration of the message handlers and MessageHandlerPr…

d2961e1

…oxy has his own interface now.

Registration of MessageHandler is now singleton as requested by @osmi…

d32522d

…rnov CityOfZion#427

Merge pull request #3 from CityOfZion/development

c703921

Increase buffer size (CityOfZion#431)

NeedPeerSync and IsSync properties added to BlockchainContext

8dd2086

MessageHandlers are type safe. The solution was to use an interface a…

3149822

…nd an abstract class. CityOfZion#427

remove example commented code

86f3e42

merge with developement

a77e4cf

aboimpinto closed this Sep 24, 2018

aboimpinto reopened this Sep 24, 2018

aboimpinto force-pushed the SyncNodeBranch branch from 7f2acfe to a77e4cf Compare September 25, 2018 00:00

Paulo Aboim Pinto added 3 commits September 25, 2018 02:03

merge with development

7319008

merge with development

f1d1e7a

merge with development

2975a70

osmirnov merged commit d97733d into CityOfZion:development Sep 25, 2018

aboimpinto deleted the SyncNodeBranch branch September 25, 2018 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync node branch #425

Sync node branch #425

aboimpinto commented Sep 20, 2018

osmirnov left a comment

osmirnov Sep 20, 2018

aboimpinto Sep 20, 2018

osmirnov Sep 20, 2018

aboimpinto Sep 20, 2018 •

edited

Loading

osmirnov Sep 20, 2018

aboimpinto Sep 20, 2018

osmirnov Sep 20, 2018

aboimpinto Sep 20, 2018 •

edited

Loading

osmirnov Sep 21, 2018

aboimpinto Sep 21, 2018

osmirnov Sep 20, 2018

aboimpinto Sep 20, 2018

osmirnov Sep 20, 2018

aboimpinto Sep 20, 2018

osmirnov Sep 20, 2018

aboimpinto Sep 20, 2018

aboimpinto Sep 20, 2018

aboimpinto Sep 20, 2018

osmirnov Sep 20, 2018

aboimpinto Sep 20, 2018

aboimpinto commented Sep 20, 2018

osmirnov Sep 21, 2018

aboimpinto Sep 21, 2018

codecov bot commented Sep 24, 2018 •

edited

Loading

aboimpinto commented Sep 25, 2018

Sync node branch #425

Sync node branch #425

Conversation

aboimpinto commented Sep 20, 2018

osmirnov 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

aboimpinto Sep 20, 2018 • 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

aboimpinto Sep 20, 2018 • 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

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

aboimpinto commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2018 • edited Loading

Codecov Report

aboimpinto commented Sep 25, 2018

aboimpinto Sep 20, 2018 •

edited

Loading

aboimpinto Sep 20, 2018 •

edited

Loading

codecov bot commented Sep 24, 2018 •

edited

Loading