I like the second idea. The more information the better when determining the cause of an error.
On Mon, Nov 6, 2017 at 12:07 PM, Mike Beckerle <[email protected]> wrote: > A reset mechanism, or automatic reset after logging the leak, seems very > useful. > > ________________________________ > From: Steve Lawrence <[email protected]> > Sent: Monday, November 6, 2017 9:14:05 AM > To: [email protected] > Subject: Issues with MarkPool not being returned > > The MarkPool currently leads to some issues that make tracking down bugs > difficult, and can also make API usage difficult. I think something > needs to change, but I'm not sure exactly what. The issue and a > potential solution is described below. > > A few places in our code create marks of the PState to support > backtracking. This includes various RepParsers, the AltCompParser, and > the ElementCombinatorParser. The contract we use for this mark pool is > that any marks that a parser takes it must also return with a call to > either discard() or reset(). At the end of a parse, we check that all > marks have been returned, and if they have not we throw an > invariantFailed(). This is considered a bug so an invariant does not > seem unreasonable. > > I believe our logic for when to return marks is correct, but it cases > where unexpected exceptions could be thrown (e.g. OutOfMemoryError), > marks are not returned. These are generally fatal exception, and so I > think that's the reason we chose to not care about them. > > However, a problem arises where an API user might want to catch what we > see as a fatal exception and try recover. For example, NiFi will catch > many exceptions, yield for some amount of time, and then retry > processing. So in the Daffodil NiFi processor, when we throw our > invariantFailed exception, NiFi catches it, yields, and then retries. > But since the mark pool is a thread local, it still contains marks that > will never be returned, and so the same invariantFailed is thrown at the > end of the next parse. NiFi will again yeild and repeat, and every > subsequent parse will fail. > > Another issue with the way this all works is that when this invariant > failed is thrown we have no idea what caused the mark to not be > returned. It may have been a fatal exception, it may have been something > else like a bug (like a ProcessingError or something that we just failed > to catch). It's very difficult to know the cause. > > So I think two things we need to resolve is: > > 1) Have some method to reset the state of the mark pool so that > processing can continue. Perhaps validateFinalState should just log an > error and then discard all existing marks. This would allow one to > continue using Daffodil if they wanted. > > 2) Have some method to determine the cause of a mark failing to be > returned, since it is possibly a bug. Perhaps we want to wrap all uses > of marks in a try/catch. The catch block could catch all exceptions and > log if there were any non-returned marks. We could also potentially > have a finally block that also returns all non-returned marks. One > downside of this is that it is possible that the returning of the mark > could throw an exception and mask the exception rethrown in the catch > block. But that does not seem that likely to me. And it does ensure the > contract of any marks taken are returned. This would also mean the above > suggestion (changing validateFinalState) is less necessary. This handles > both the logging of exceptions that would cause a mark to not be > returned, and forces returning of the mark. > > Thoughts? > -- -Taylor Wise
