[
https://issues.apache.org/jira/browse/DAFFODIL-2510?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17334325#comment-17334325
]
Mike Beckerle edited comment on DAFFODIL-2510 at 4/27/21, 11:27 PM:
--------------------------------------------------------------------
The Logging trait is mixed into many places. It has mutable state for assigning
a logger and assigning a log level.
This is wrong. Mixing in Logging should allow code to make logging calls. But
should not necesarily give the object its own logger state that
can be set. Mixin in Logging should give you the methods to do log calls, and
a undefined virtual method:
{code:java}
def logger: Logger{code}
which has to get defined somehow - passed to constructor or delegated to a
related object (such as enclosing parent).
This Logger is the mutable object, which defaults to using the LoggingDefaults.
By default the logWriter and logLevel vars get their values from the
LoggingDefaults object, a global singleton which also has mutable state for
logLevel and logWriter.
One quick fix is perhaps to just make logWriter and logLevel in
LoggingDefaults thread locals.
There are 38 uses of the Logging trait where it is extended or mixed in (via
the extends XYZ with Logging).
Certainly all these places have, or potentially have, the need to write to the
log.
What they don't all need is their own settable logger state.
I looked at each of the 38.
* daffodil-cli has 3: Main, CLIConf, and CommandLindSAXErrorHandler all mix
Logging in. Only Main should. Everything else should be passed that one.
* daffodil-core has 11:
** Of these 8 are mixed into test classes like TestExternalVariablesNew. This
makes no sense. Test code doesn't need to make logging calls. It does have to
pass a logger to things it is testing, but that's different from mixing in the
Logging trait.
** The 3 that are not in test code are in Compiler, ChainPropProvider and
LeafPropProvider. A LeafPropProvider is itself a mixin to a DSOM object, so it
should be inheriting a logger from the Compiler as a whole. The
ChainPropProvider is a concrete class constructed by DFDLFormatAnnotation so
can be passed the Compiler's logger.
** So the whole schema compiler should have 1 logger, and that should come
from the calling application doing the compiling (such as CLI Main).
* daffodil-io has 3.
** This is runtime system code, and is per-thread, not shared by threads, so
should be inheriting the thread's logger.
** I.e., a logger from PState/UState, or the PState/UState, and the
io.DataInputStream and io.DataoutputStream objects should all be constructed
and given the same per-thread logger.
* daffodil-lib has 11. 2 are in TestLogger, leaving 9.
** DaffodilTunables 1 - should be passed one at construction.
** OOLAG (3) shoulld be getting them from the compiler data structures these
are mixed into.
** PropertyMixin (1) - same thing
** Timer (2) - these are probably ok. This is a timer utility. Singleton
objects.
** DFDLCatalogResolver - should be getting it from the compiler or runtime
depending on who is using it (used at runtime by Xerces validation...)
** DFDLXercesAdapter - ditto (we use Xerces at compile time as well as for
validation at runtime)
* daffodil-runtime1 has 6:
** LengthState (1) - needs to log because these length calculations at runtime
are a subtle place for deadlocks, but should be using the PState/Ustate logger.
** Processor (1) - This one should come from whatever is creating the
processor (ProcessorFactory, or Compiler restoring a processor from serialized
form), but methods like parse and unparse should be constructing their own
per-thread logger.
** ParseOrUnparseState(1) - this is the primary logger for the runtime1. Most
things at runtime should be getting this logger and using it.
** Suspension(1) - should be getting from UState
** SuspensionTracker(1) - should be getting from UState
** UserDefinedFunctionService - This one is trickier. I think this should be
getting it from the compiler or runtime depending on who is using it
* daffodil-tdml-lib has 3: We run tests in parallel under sbt, so this is
trickier.
** DFDLTestSuite(1) - makes sense. Should be passed to constructor.
** TestCase - should inherit from the test suite?
** FileDocumentPart - should inherit from the TestCase.
* daffodil-test has 1: TestNamspacesNew - this makes no sense.
was (Author: mbeckerle):
The Logging trait is mixed into many places. It has mutable state for assigning
a logger and assigning a log level.
This is wrong. Mixing in Logging should allow code to make logging calls. But
should not necesarily give the object its own logger state that
can be set. Mixin in Logging should give you the methods to do log calls, and a
undefined virtual method:
{code:java}
def logger: Logger{code}
which has to get defined somehow - passed to constructor or delegated to a
related object (such as enclosing parent).
This Logger is the mutable object, which defaults to using the LoggingDefaults.
By default the logWriter and logLevel vars get their values from the
LoggingDefaults object, a global singleton which also has mutable state for
logLevel and logWriter.
One quick fix is perhaps to just make logWriter and logLevel in LoggingDefaults
thread locals.
There are 38 uses of the Logging trait where it is extended or mixed in (via
the extends XYZ with Logging).
Certainly all these places have, or potentially have, the need to write to the
log.
What they don't all need is their own settable logger state.
I looked at each of the 38.
* daffodil-cli has 3: Main, CLIConf, and CommandLindSAXErrorHandler all mix
Logging in. Only Main should. Everything else should be passed that one.
* daffodil-core has 11:
** Of these 8 are mixed into test classes like TestExternalVariablesNew. This
makes no sense. Test code doesn't need to make logging calls. It does have to
pass a logger to things it is testing, but that's different from mixing in the
Logging trait.
** The 3 that are not in test code are in Compiler, ChainPropProvider and
LeafPropProvider. A LeafPropProvider is itself a mixin to a DSOM object, so it
should be inheriting a logger from the Compiler as a whole. The
ChainPropProvider is a concrete class constructed by DFDLFormatAnnotation so
can be passed the Compiler's logger.
** So the whole schema compiler should have 1 logger, and that should come
from the calling application doing the compiling (such as CLI Main).
* daffodil-io has 3. This is runtime system code, and is per-thread, not
shared by threads, so should be inheriting the thread's logger. I.e., a logger
from PState/UState, or the PState/UState, and the io.DataInputStream and
io.DataoutputStream objects should all be constructed and given the same
per-thread logger.
* daffodil-lib has 11. 2 are in TestLogger, leaving 9.
** DaffodilTunables 1 - should be passed one at construction.
** OOLAG (3) shoulld be getting them from the compiler data structures these
are mixed into.
** PropertyMixin (1) - same thing
** Timer (2) - these are probably ok. This is a timer utility. Singleton
objects.
** DFDLCatalogResolver - should be getting it from the compiler or runtime
depending on who is using it (used at runtime by Xerces validation...)
** DFDLXercesAdapter - ditto (we use Xerces at compile time as well as for
validation at runtime)
* daffodil-runtime1 has 6:
** LengthState (1) - needs to log because these length calculations at runtime
are a subtle place for deadlocks, but should be using the PState/Ustate logger.
** Processor (1) - This one should come from whatever is creating the
processor (ProcessorFactory, or Compiler restoring a processor from serialized
form), but methods like parse and unparse should be constructing their own
per-thread logger.
** ParseOrUnparseState(1) - this is the primary logger for the runtime1. Most
things at runtime should be getting this logger and using it.
** Suspension(1) - should be getting from UState
** SuspensionTracker(1) - should be getting from UState
** UserDefinedFunctionService - This one is trickier. I think this should be
getting it from the compiler or runtime depending on who is using it
* daffodil-tdml-lib has 3: We run tests in parallel under sbt, so this is
trickier.
** DFDLTestSuite(1) - makes sense. Should be passed to constructor.
** TestCase - should inherit from the test suite?
** FileDocumentPart - should inherit from the TestCase.
* daffodil-test has 1: TestNamspacesNew - this makes no sense.
> Thread Safety: Improve Logger/Logging
> --------------------------------------
>
> Key: DAFFODIL-2510
> URL: https://issues.apache.org/jira/browse/DAFFODIL-2510
> Project: Daffodil
> Issue Type: Bug
> Components: Back End, CLI, Front End, Libraries, Middle
> "End", TDML Runner
> Affects Versions: 3.0.0
> Reporter: Mike Beckerle
> Priority: Major
>
> Every object that mixes in the Logging trait gets its own mutable vars for
> logWriter and logLevel, and if not set, shares a single global state in the
> LoggingDefaults object.
> This is not thread safe. At minimum LoggingDefaults should be a ThreadLocal.
> Furthermore, those objects don't need independent ability to specify a unique
> logger or logging level. They should be sharing a logger supplied by the
> application.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
