Skip to content

Conversation

@majocha
Copy link
Contributor

@majocha majocha commented Feb 23, 2024

Description

There was some odd behavior I noticed while trying to test AsyncLocal diagnostics state.
UseDiagnosticsLogger creates a diagnostics scope and should restore previous logger on dispose. This should in theory work consistently, pushing and unwinding loggers in the given order. I noticed the disposals often happen out of order, leading to momentarily inconsistent state.

By chance I stumbled upon what I believ are some slight bugs in ParseAndCheckInputs.fs:

/// Typecheck a single file (or interactive entry into F# Interactive)
let CheckOneInputEntry (ctok, checkForErrors, tcConfig: TcConfig, tcImports, tcGlobals, prefixPathOpt) tcState input =
// Equip loggers to locally filter w.r.t. scope pragmas in each input
use _ =
UseTransformedDiagnosticsLogger(fun oldLogger -> DiagnosticsLoggerForInput(tcConfig, input, oldLogger))
use _ = UseBuildPhase BuildPhase.TypeCheck
RequireCompilationThread ctok
CheckOneInput(checkForErrors, tcConfig, tcImports, tcGlobals, prefixPathOpt, TcResultsSink.NoSink, tcState, input)
|> Cancellable.runWithoutCancellation

This function returns a cancellable, so the disposables will get disposed immediately instead of participating in the cancellable execution.

Similar situation here:

let processFile
(node: NodeToTypeCheck)
((input, logger): ParsedInput * DiagnosticsLogger)
((currentTcState, _currentPriorErrors): State)
: Finisher<NodeToTypeCheck, State, PartialResult> =
use _ = UseDiagnosticsLogger logger
let checkForErrors2 () = priorErrors || (logger.ErrorCount > 0)
let tcSink = TcResultsSink.NoSink
let (Finisher(finisher = finisher)) =
CheckOneInputWithCallback
node
(checkForErrors2, tcConfig, tcImports, tcGlobals, prefixPathOpt, tcSink, currentTcState, input, false)
|> Cancellable.runWithoutCancellation
Finisher(
node,
(fun (state: State) ->
let tcState, priorErrors = state
let (partialResult: PartialResult, tcState) = finisher tcState
let hasErrors = logger.ErrorCount > 0
let priorOrCurrentErrors = priorErrors || hasErrors
let state: State = tcState, priorOrCurrentErrors
partialResult, state)
)

Disposal happens immediately, leaving us with other global diagnostic logger than expected.

Let's see if fixing this breaks something else.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 23, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@majocha majocha marked this pull request as ready for review February 23, 2024 10:52
@majocha majocha requested a review from a team as a code owner February 23, 2024 10:52
@majocha
Copy link
Contributor Author

majocha commented Feb 23, 2024

This is only tangentially related, but I replaced the instances where we don't intend to ever unwind:

let _holder = UseDiagnosticsLogger diagnosticsLogger

with

SetThreadDiagnosticsLoggerNoUnwind diagnosticsLogger

which does not allocate a disposable and makes it easier to later test for correct order of unwinds.

Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this moves us to the safer side. Thanks!

@psfinaki
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Mar 18, 2024
@T-Gro T-Gro enabled auto-merge (squash) March 18, 2024 14:20
@T-Gro T-Gro merged commit 9c2bb4b into dotnet:main Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants