error channel is not consumed

[andaru]

Issue #71 identifies two problems. The first, as that issue describes, is related to escape character handling.

The second, shown in the error message provided in #71 is a deadlock, which occurs due to the lexer's items channel overflowing.

This issue is opened to address that part of the problem.

At present, errors can be sent to the items channel an arbitrary number of times between each call to (*lexer).NextToken(), which is the only place which receives from the items channel. Each call to NextToken will remove at most one item. When multiple errors are detected (in issue #71) within the same comment, only a single NextToken call will occur, allowing the items channel to overflow in short order.

[andaru]

Diagnosis:

The stack path causing the issue in #71 comes down to the lexQString function's inner for {} loop, specifically this code.

if !l.inPattern {
	l.ErrorfAt(line, col, `invalid escape sequence: \`+string(c))
}
text = append(text, '\\')

After this executes, the for loop continues, which due to there being no goroutine consuming the items channel, allows for the channel to fill up and the deadlock to occur.

An elegant solution might be to start a goroutine to consume the channel prior to lexQString ever being called.

Item consumption occurs via calling NextToken manually. This is presently called only from within (*parser).next(), itself only called from nextStatement which it called by itself recursively and ultimately from Parse.

The current control flow does not immediately lend itself to setting up a goroutine to consume the channel, but an elegant approach to that may appear given a little more consideration.

The simple approach of exiting NextToken as soon as any error is seen would work, but will make the parser strictly "first error" only, which is less than ideal considering multiple errors are supported today, including up to [size of items channel] consecutive errors between other tokens.

Given this issue has existed in the library since release, I'll keep working on a more elegant approach with a goroutine started by the call to Parse.

[pborman]

You can put a select where the message is written and indicate a fatal error if the default case occurs (since it only happens after repeated failures). Just a thought.

…

Sent from my iPad

On Apr 18, 2020, at 12:49 AM, Andrew Fort ***@***.***> wrote:  Diagnosis: The stack path causing the issue in #71 comes down to the lexQString function's inner for {} loop, specifically this code. if !l.inPattern { l.ErrorfAt(line, col, `invalid escape sequence: \`+string(c)) } text = append(text, '\\') After this executes, the for loop continues, which due to there being no goroutine consuming the items channel, allows for the channel to fill up and the deadlock to occur. An elegant solution might be to start a goroutine to consume the channel prior to lexQString ever being called. Item consumption occurs via calling NextToken manually. This is presently called only from within (*parser).next(), itself only called from nextStatement which it called by itself recursively and ultimately from Parse. The current control flow does not immediately lend itself to setting up a goroutine to consume the channel, but an elegant approach to that may appear given a little more consideration. The simple approach of exiting NextToken as soon as any error is seen would work, but will make the parser strictly "first error" only, which is less than ideal considering multiple errors are supported today, including up to [size of items channel] consecutive errors between other tokens. Given this issue has existed in the library since release, I'll keep working on a more elegant approach with a goroutine started by the call to Parse. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[andaru]

Thanks, that's a good idea to replace the deadlock with an explicit crash. I'll do that here.

Do you think it's worth looking at cleaning this up? We have a channel (looks like it comes from Rob's lexer/parser example, a pattern I quite like..), plus a stack of items on the receiver side. Since the channel send and receive calls happen in the same stack (frame), we could eliminate the channel itself as there are no goroutines to co-ordinate. Or we could refactor the receiver to properly run in its own goroutine.