Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Refactoring] Remove the backend global singleton #1824

Merged

Conversation

@NicolasDorier
Copy link
Collaborator

commented Jul 7, 2019

This is needed for #1815

I remove the backend global singleton by initializing it as part of the Startup and registering it in the service collections of the website so it can be injected where needed.

This has the nice side effect to effectively test the initialization code that was previously in Program.cs.
I needed to add ListenRoundConfigFileChanges and disable it for the tests to pass.

Also I removed some mess to detect the wwwroot directory and indeed just use HostingEnvironment.WebRootPath provided by asp.net core.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/remove-backend-singleton branch from f6d82bb to 92b0a7f Jul 7, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

Idea ACK. Before code review, let me ask, is it the same pattern you used in the Gui side?

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

Yes, but actually it is simpler than the Gui side, because Avalonia IoC system is a big hack, while ASP.NET Core is made for IoC. This mean that most of the classes which requires Global to be passed in argument now are automatically injected the Global without having to code for it.

@nopara73
Copy link
Collaborator

left a comment

The code looks good, except my change requests and that you added ListenRoundConfigFileChanges. I couldn't figure out any reason why the tests fail without it, other than you changed the behavior somehow.
I also didn't test it, so that's still left to do.

Merge ToDo list:

  • Remove or discuss ListenRoundConfigFileChanges
  • Make changes requested.
  • Run CodeMaid through all the files.
  • Make sure tests pass.
  • Test the behavior.
@@ -43,6 +44,10 @@ public class Config : IConfig
[JsonProperty(PropertyName = "RegTestBitcoinCorePort")]
public int? RegTestBitcoinCorePort { get; internal set; }

[DefaultValue(true)]
[JsonProperty(PropertyName = "ListenRoundConfigFileChanges", DefaultValueHandling = DefaultValueHandling.Populate)]
public bool ListenRoundConfigFileChanges { get; set; }

This comment has been minimized.

Copy link
@nopara73

nopara73 Jul 8, 2019

Collaborator

Please follow the same patterns used here with this new configuration entry.

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Jul 8, 2019

Author Collaborator

What do you mean? it is the same pattern.

This comment has been minimized.

Copy link
@nopara73

nopara73 Jul 9, 2019

Collaborator

We don't use [DefaultValue(true)] here yet.

WalletWasabi.Backend/Config.cs Outdated Show resolved Hide resolved
WalletWasabi.Backend/Controllers/BatchController.cs Outdated Show resolved Hide resolved
WalletWasabi.Backend/UnversionedWebBuilder.cs Outdated Show resolved Hide resolved
WalletWasabi.Backend/Startup.cs Outdated Show resolved Hide resolved
WalletWasabi.Backend/Startup.cs Outdated Show resolved Hide resolved
@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

@nopara73 about ListenRoundConfigFileChanges, the reason is very simple.

I moved a bunch of code out of the Program.cs into the StartupTask. Part of this code is listening the config file changes.

In the regtestsfixture, you had your own boilerplate, similar to what was in the Program.cs but without the listening of config file change.

When I moved all the initialization code out of Program.cs and RegtestFixture, and inside the StartupTask, now the Regtests were also listening config file changes.

However, this conflict with the tests. Because the tests change the config at runtime, and the listener would see that the value at runtime is different from the file and reload the file.

An alternative is to get rid of the runtime's UpdateConfigRound and directly edit the file. This would have the advantage to also test the config listener.

Btw, I find it weird you don't use .NET FileSystemWatcher

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/remove-backend-singleton branch 3 times, most recently from 75ff55d to 6cbe42d Jul 8, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

Rebased following the changes you suggested.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

Thanks @NicolasDorier I take this from here.

Your reasoning of ListenRoundConfigFileChanges makes sense.

I find it weird you don't use .NET FileSystemWatcher

Never heard of it, but I'll keep it in mind. It sounds very useful. @molnard take a note, too.

An alternative is to get rid of the runtime's UpdateConfigRound and directly edit the file. This would have the advantage to also test the config listener.

Yes, that's the clean way to do this. I'll implement that.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

I removed ListenRoundConfigFileChanges and ToFile-ing for the CcjRoundConfig modifications. This is less intrusive change.

However this PR has multiple severe issues.

1

If I run this PR on the RegTest, then the swagger doesn't work.

image

2

Running the the RegTets puts trash in the Config.cs

{
  "Network": "RegTest",
  "BitcoinRpcConnectionString": "server=http://127.0.0.1:11557/;d001e8490cd5428eb7a1a009f6ea442c3097e890:d001e8490cd5428eb7a1a009f6ea442c3097e890",
  "MainNetBitcoinCoreHost": "127.0.0.1",
  "TestNetBitcoinCoreHost": "127.0.0.1",
  "RegTestBitcoinCoreHost": "127.0.0.1",
  "MainNetBitcoinCorePort": 8333,
  "TestNetBitcoinCorePort": 18333,
  "RegTestBitcoinCorePort": 10787
}

It messes up the server entry and the RegTestBitcoinCorePort gets some strange random value.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

@nopara73 it is not garbage.

This PR is making sure that each tests are isolated by creating a separate regtest node for each test, both ports are chosen randomly.

  • You can run test concurrently just fine
  • The tests will be more deterministic

The swagger thing is strange, will take a look after rebasing.

BitcoinRpcConnectionString is a proper connection string as wanted by the RPCClient. Before you were using default.


Edit: Actual you were always using a new node... for p2p, but not for RPC oO

NicolasDorier and others added 6 commits Jul 7, 2019

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/remove-backend-singleton branch from b8c30d6 to d3d689f Jul 10, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

I rebased and cleaned up the history a bit.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

@nopara73 I know why it is like that. Because the fixture and tests were communicating via the shared global instance before. I changed that by saving into file, but I should not have done that way. Fixing it.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

@nopara73 so I found out why I did this actually.
Because now the Startup is responsible for creating the global instance, you can't just create from outside. So the way I worked around this, is by changing the config file before starting the host.

But because your node builder is a singleton, it is using the %APPDATA% one.
So it means that I can't fix this without also removing the test singleton issues.

Another way would be to use NBitcoin.TestFramework for the creation of Nodes. It does not suffer from this problem as it does not rely on singletons.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

wait actually I have another idea to fix it.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

@nopara73 sorry for that, it was easy to fix thanks to this PR actually: Just have to tell to Global to not use the user's config directory and instead create one for the tests. It works nice now.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

Anything missing now?

nopara73 added 2 commits Jul 11, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

Nicolas, I still get the swagger error. Was that supposed to be fixed by now?

  • Up to date with the latest master.
  • CodeFactor
  • CodeMaid
  • CI
  • Tests pass.
  • Tests don't ruin the config.
  • Swagger error fixed.
  • Test behavior.
  • Review code.
@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

@nopara73 I think the swagger is because I trahsed your config in %APPDATA%.

Will check now.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

Just fixed swagger, I don't think it comes from this PR though as you can see.

@@ -50,14 +58,15 @@ public void ConfigureServices(IServiceCollection services)
services.AddLogging(Logging => Logging.AddFilter((s, level) => level >= Microsoft.Extensions.Logging.LogLevel.Warning));

services.AddSingleton<IExchangeRateProvider>(new ExchangeRateProvider());

services.AddSingleton<Global>(new Global(Configuration["datadir"]));

This comment has been minimized.

Copy link
@lontivero

lontivero Jul 11, 2019

Collaborator

Why do we need getting the datadir from a different config file? Is it for testing?

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Jul 12, 2019

Author Collaborator

yes, so Regtests does not use the same config file than the user's datadir.

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

I've reviewed it and everything looks good and pretty clear but it is a big PR. How should we test it?

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

@lontivero run the tests, and run the daemon.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

Can somebody kick in the box to make wasabi.osx green?

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

Swagger works okay. Tests run okay most of the times (this is not new in my env). The error that I find weird running the tests is this one:

2019-07-12 13:24:56 DEBUG IndexBuilderService: NBitcoin.RPC.RPCException: Block height out of range
   at NBitcoin.RPC.RPCResponse.ThrowIfError()
   at NBitcoin.RPC.RPCClient.SendCommandAsyncCore(RPCRequest request, Boolean throwIfRPCError)
   at NBitcoin.RPC.RPCClient.SendCommandAsync(RPCRequest request, Boolean throwIfRPCError)
   at NBitcoin.RPC.RPCClient.GetBlockHashAsync(Int32 height)
   at NBitcoin.RPC.RPCClient.GetBlockAsync(Int32 height)
   at WalletWasabi.Services.IndexBuilderService.<Synchronize>b__35_0() in /home/lontivero/GitHub/WalletWasabi/WalletWasabi/Services/IndexBuilderService.cs:line 261

I didn't see this error before.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 13, 2019

@lontivero I think later I will make each of those tests run a separate bitcoin node. This is probably why the tests are non deterministic.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

I will give another round of test and code review to this PR, if it won't be sufficient, I'll apply our new refactoring guidelines here: https://github.com/zkSNACKs/WalletWasabi/blob/master/CONTRIBUTING.md#refactoring

nopara73 added 5 commits Jul 15, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

@molnard and me spent 2-3 hours to carefully review this together again. We are 99% sure that it won't result in unexpected issues. Merging.

@nopara73 nopara73 merged commit 29dc0aa into zkSNACKs:master Jul 15, 2019

3 of 4 checks passed

Wasabi.Windows in progress
Details
CodeFactor No issues found.
Details
Wasabi.Linux #20190715.15 succeeded
Details
Wasabi.Osx #20190715.15 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.