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

Failed to add json file when calling GetConfiguration() #1531

Closed
nicolegys opened this issue Apr 3, 2020 · 8 comments · Fixed by #1564
Closed

Failed to add json file when calling GetConfiguration() #1531

nicolegys opened this issue Apr 3, 2020 · 8 comments · Fixed by #1564
Assignees

Comments

@nicolegys
Copy link
Contributor

Describe the bug
I tried to update the config file of RpcServer while neo-cli was running, but sometimes the neo-cli would throw exception and crash.
image

To Reproduce
Steps to reproduce the behavior:

  1. start neo-cli.
  2. change some settings in the config file of RpcServer, such as DisabledMethods, then save it.
  3. most of the time, neo-cli will throw the exception and crash.

Platform:

  • OS: [e.g. Windows 10 x64]
  • Version
    neo-cli: 5a46401b3635b0f2f6a6b09141c20ea5ab361623
    neo: 6eba2ee
    neo-vm: f292fed3e9add32a2746e2c4cbb569d8dce6f25a
    neo-modules: bbcfa56171a931c1c6f2b50f9289350dd7c052c0

(Optional) Additional context
It seems that the json file wasn't added successfully.
image
When debugging, if I set a breakpoint, then continue, the file can be added successfully and not throw an excepiton.
image

The issue was found when testing neo-project/neo-modules#206

@Qiao-Jin
Copy link
Contributor

Qiao-Jin commented Apr 3, 2020

I will have a look at this one.

@erikzhang
Copy link
Member

I think the real reason is that WatcherChangeTypes.Changed will be triggered multiple times. Maybe we can just catch the exception and ignore it in the plugins.

@nicolegys
Copy link
Contributor Author

I think the real reason is that WatcherChangeTypes.Changed will be triggered multiple times. Maybe we can just catch the exception and ignore it in the plugins.

You're right. When the config file is modified by editors like Notepad, the WatcherChangeTypes.Changed will be triggered once, but when modified by editors like VS code, the WatcherChangeTypes.Changed will be triggered twice, and GetConfiguration() will fail at the first time.

@erikzhang
Copy link
Member

Then the fixes should be in the plugins.

@Qiao-Jin
Copy link
Contributor

Qiao-Jin commented Apr 7, 2020

Then the fixes should be in the plugins.

Then we would need duplicate logic added everywhere func GetConfiguration called in plugins (config file changed, initialization)?

@shargon
Copy link
Member

shargon commented Apr 7, 2020

Agree with @Qiao-Jin , I think that it's better to fix it in the core and avoid duplicate code in modules.

@erikzhang
Copy link
Member

Look at this:

private static void ConfigWatcher_Changed(object sender, FileSystemEventArgs e)
{
switch (GetExtension(e.Name))
{
case ".json":
Plugins.FirstOrDefault(p => p.ConfigFile == e.FullPath)?.Configure();
break;
case ".dll":
if (e.ChangeType != WatcherChangeTypes.Created) return;
if (GetDirectoryName(e.FullPath) != PluginsDirectory) return;
try
{
LoadPlugin(Assembly.Load(File.ReadAllBytes(e.FullPath)));
}
catch { }
break;
}
}

The event is triggered when file is changed. Then it invokes the method Plugin.Configure(). Be careful, it is Configure(), not the GetConfiguration().

And the default Configure() is:

protected virtual void Configure()
{
}

As you can see, it does nothing. So you didn't fix anything if you catch the exception in GetConfiguration().

The most logical way is to call the GetConfiguration() in the plugin's overriding Configure(), which has already been done. And we just add some code around it to catch the exception.

@Qiao-Jin
Copy link
Contributor

Qiao-Jin commented Apr 8, 2020

@nicolegys please have a test of the new PR:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants