Skip to content

Logging level is set after features are created and initialized, not before #1378

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

Closed
glennsarti opened this issue Jun 21, 2018 · 6 comments
Closed
Assignees
Labels

Comments

@glennsarti
Copy link
Contributor

System Details

  • Operating system name and version: Windows 10 - 1803
  • VS Code version: 1.24.0
  • PowerShell extension version: Commit ddc75b8
  • Output from $PSVersionTable: N/A

Issue Description

While working on a new feature for the extension, I noticed that I was unable to call writeError etc. on the logger object during Feature initialisation. I found this was due to a race-ish condition.

  1. We create the logging object here

logger = new Logger();

  1. We then create the features here

// Create features
extensionFeatures = [
new ConsoleFeature(),
new ExamplesFeature(),
new OpenInISEFeature(),
new GenerateBugReportFeature(sessionManager),
new ExpandAliasFeature(),
new ShowHelpFeature(),
new FindModuleFeature(),
new PesterTestsFeature(sessionManager),
new ExtensionCommandsFeature(logger),
new SelectPSSARulesFeature(),
new CodeActionsFeature(),
new NewFileOrProjectFeature(),
new DocumentFormatterFeature(logger, documentSelector),
new RemoteFilesFeature(),
new DebugSessionFeature(context, sessionManager),
new PickPSHostProcessFeature(),
new SpecifyScriptArgsFeature(context),
new HelpCompletionFeature(logger),
new CustomViewsFeature(),

  1. However, the debug log level is is only set at;

this.log.startNewLog(this.sessionSettings.developer.editorServicesLogLevel);

  1. Which is called from;
    sessionManager.start();

which is AFTER the features are created.

The race condition comes from how node schedules things on threads. Sometimes it was set in time for the feature creation, sometimes not.

@glennsarti
Copy link
Contributor Author

Perhaps the log level code should be moved from the session manager into the logger itself.

@rkeithhill
Copy link
Contributor

Well, except that at some point you want to set the log level to the setting the user may have set in their user/workspace settings file. That setting value can only be read after the SessionManager has started. What we could do is start logging with a default log-level of say normal and then when we get into the SessionManager.Start() method, have it set the log level to what was specified by the settings.

@glennsarti
Copy link
Contributor Author

glennsarti commented Jun 21, 2018

That's exactly what it's doing right now.

And that's the racey bit. As a feature you don't know when the loglevel is set correctly. So should I call logger.write or logger.writeVerbose. It's a little unclear.

Also the log level is coming from the workspace/user settings, so there's nothing really "Session-y" about it.

@rkeithhill
Copy link
Contributor

I suppose we could have the logger do a Settings.load() to get the desired log level. If that works then we're set. Just start logger right after we create the Logger.

@glennsarti
Copy link
Contributor Author

glennsarti commented Jun 21, 2018

Yup, just move the start call out of the session Manager and into main.ts

OR

allow the logger to have .start called idempotently

OR

have a property on the logger to indicate it has already started

@rkeithhill rkeithhill self-assigned this Jul 11, 2018
@rkeithhill
Copy link
Contributor

This was fixed in the 1.8.0 release.

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

No branches or pull requests

3 participants