-
Notifications
You must be signed in to change notification settings - Fork 461
[Bug] Do not try to initialize the TableClient if the service is disabled #11195
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
Conversation
src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs
Show resolved
Hide resolved
{ | ||
_flushLogsTimer?.Value?.Dispose(); | ||
} | ||
|
||
FlushLogs().GetAwaiter().GetResult(); | ||
if (_tableClient is not null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like an abuse to reference the field here, when the intent of the TableClient property is to ensure all usages go through the property.
Perhaps Dispose should just set _isEnabled to false, or call DisableService, which would cause FlushLogs to noop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did originally start with _isEnabled = false
but we opted not to use that approach because of Fabio's comment: #11195 (comment)
This introduces a change in behavior and potential data loss if this was running properly (a client was already created) and logs were buffered.
The TableClient
propery is overloaded and we have another issue to refactor the whole thing. This is patch in the meantime to address this bug #11194
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, not an ideal pattern/fix, but I'm OK with us doing this if we're tracking a longer-term change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented above. I'm OK with the change, but let's have a test in place to validate the behavior with the changes in place.
Discussed with Fabio and as we already have a solution for the core tools problem, we will close this PR in favour of a fixing the real issue with the TableClient property: |
Issue describing the changes in this PR
resolves #11194
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-proc
branch to be included in Core Tools and non-Flex deployments.in-proc
branch is not requiredrelease_notes.md
Additional information
Do not try to initialize the TableClient if the service is disabled.