Skip to content

Instantly share code, notes, and snippets.

@akavel akavel/01.md
Last active Oct 31, 2018

Embed
What would you like to do?

Converting fragments of real code (with error handling) to Go 2 "design draft" syntax

Part 1: A successful fragment

Following the announcement of Go 2 design drafts related to error handling (a.k.a. "if err != nil { return err }" pattern), I tried to convert a fragment of real nontrivial code (with some censoring) to the new tentative syntax. (Originally I remembered this code as particularly tricky, but I think it was rewritten in the meantime to more straightforward, so it has less impact for me than I thought it would. Still, this is some real code full well.)

The old code looked like below:

func (t *X) initializeAndActivate(ctx context.Context) error {
t.logger.Info(fmt.Sprintf("activating X for transaction [%q] @%s", xxx.TransactionCodeFrom(ctx), t.millis()))
t.bus.Lock()
defer t.bus.Unlock()
if t.fooing {
return fmt.Errorf("trying to activate X for transaction %q while already active", xxx.TransactionCodeFrom(ctx))
}
response, err := t.queryOnly(cmd_foo1)
if err != nil {
t.hardReset()
return err
}
if response[0] != cmd_foo1[0] {
t.hardReset()
return fmt.Errorf("invalid response to FOO1 [% 02X]", response)
}
t.fooTable, err = parseFooTable(response[1:])
if err != nil {
t.hardReset()
return err
}
t.logger.Info(fmt.Sprintf("X parsed foo table [% 02x] as: %s", response, yyy.JsonString(t.fooTable)))
err = t.queryAndExpect(cmd_foo2, cmd_foo2)
if err != nil {
t.hardReset()
return err
}
err = t.queryAndExpect(cmd_foo3, cmd_foo3)
if err != nil {
t.hardReset()
return err
}
err = t.queryAndExpect(cmd_foo4, cmd_foo4)
if err != nil {
t.hardReset()
return err
}
t.logger.Info(fmt.Sprintf("activated X for transaction [%q] @%s", xxx.TransactionCodeFrom(ctx), t.millis()))
t.fooing = true
return nil
}

Based on my understanding of the design draft, the new code could probably look like below:

func (t *X) initializeAndActivate_Go2(ctx context.Context) error {
t.logger.Info(fmt.Sprintf("activating X for transaction [%q] @%s", xxx.TransactionCodeFrom(ctx), t.millis()))
t.bus.Lock()
defer t.bus.Unlock()
if t.fooing {
return fmt.Errorf("trying to activate X for transaction %q while already active", xxx.TransactionCodeFrom(ctx))
}
handle err {
t.hardReset()
return err
}
response := check t.queryOnly(cmd_foo1)
if response[0] != cmd_foo1[0] {
check fmt.Errorf("invalid response to FOO1 [% 02X]", response)
}
t.fooTable = check parseFooTable(response[1:])
t.logger.Info(fmt.Sprintf("X parsed foo table [% 02x] as: %s", response, yyy.JsonString(t.fooTable)))
check t.queryAndExpect(cmd_foo2, cmd_foo2)
check t.queryAndExpect(cmd_foo3, cmd_foo3)
check t.queryAndExpect(cmd_foo4, cmd_foo4)
t.logger.Info(fmt.Sprintf("activated X for transaction [%q] @%s", xxx.TransactionCodeFrom(ctx), t.millis()))
t.fooing = true
return nil
}

Observations

Notably, the block in lines 15-17 was quite interesting. At first, I left it as:

if response[0] != cmd_foo1[0] {
	t.hardReset()
	return fmt.Errorf("invalid response to FOO1 [% 02X]", response)
}

At this point, the t.hardReset() fragment was somewhat standing out in the flow of code, feeling like something that I could easily forget to call when writing the code from scratch.

But then I remembered of seeing a check err pattern used somewhere in the design draft, and realized I could use this here. The resulting code is even more surprising. I like that it introduces some reuse, and hides the t.hardReset() as a "default behavior". At the same time, I dislike two things about it:

  • that the block is missing an explicit return, so I don't easily see it's a 100% sure exit point from the function;
  • that the line seems to read weirdly in English: check Error("some message")?... there's nothing to check here, there's just an error to return (and t.hardReset() to be called). (That said, try fmt.Errorf(...) would have the same issue.)

Thoughts

Personally, I'm not 100% sure what I think of the change.

Certainly, the code looks shorter and a bit more terse; no more "if err != nil" boilerplate. The block doing additional (substantial) verification in l.15-17 now stands out more, which seems a good thing.

On the other hand, the "handle" block looks a bit weird. Now that I think of it, it somehow feels to be missing symmetry with defer blocks syntactically — while being most similar to defer blocks semantically (more so than to if, switch, for, etc. — as "handle" is not immediate as these are).

Also, I don't like that it feels like I have to do a bit more jumping around in the code with my eyes, and/or carry more context in memory when reading and analyzing it (namely, contents of the handle block).

But again on a positive note, I can worry less if I haven't made a typo in one of the "if err != nil" blocks. Any outstanding behaviors from the default handle contents would have to be made explicit now.

Part 2: A failed attempt on another fragment of code

I found another fragment of code with non-trivial error handling. However, I didn't manage to find a reasonable approach how to apply handle & check to "if err != nil" blocks there. I don't know if it's bad or good, just reporting. (There are two places with "if err != nil": l. 13 and l. 27. Both of them seem to add significant new context to the returned message.) Maybe the "error values" design draft could help here somehow?

func (b *Bus) readAndUnpack() ([]byte, error) {
	n, err := io.ReadAtLeast(b.port, b.buf[:], 2)
	got := b.buf[:n]
	if err != nil {
		return nil, newError("xxx: cannot read 2-byte preamble [got: % 02X] - error: %s", got, err)
	}
	if got[0] != magic_number {
		return nil, newError("xxx: bad MAGIC NUMBER in response - message starts with: [% 02X] (expected XX...)", got)
	}
	length := int(got[1])
	if length < 5 {
		return nil, newError("xxx: response too short: len=%d < 5 [% 02X]", length, got)
	}

	// Now that we know the total length, we can read the remaining bytes of the response
	if n < length {
		n, err = io.ReadAtLeast(b.port, b.buf[n:], length-n)
		if err != nil {
			return nil, newError("xxx: cannot read remaining bytes of a packet [prefix=% 02X][rest=% 02X] - error: %s",
				got, b.buf[len(got):][:n], err)
		}
		got = b.buf[:len(got)+n]
	}
	crc := crc(got[:length-2])
	if crc != [2]byte{got[length-2], got[length-1]} {
		return nil, newError("xxx: bad CRC [% 02X], expected [...% 02X]", got, crc[:])
	}

	payload := append([]byte(nil), got[2:length-2]...)
	return payload, nil
}

Part 3: Comparative analysis

One extra detail I realized I forgot to mention before, is that the first example uses an error library based on log15.v2. This is important because the errors created with this library automatically collect stack trace information. This makes the return err lines so trivial in the original example. And apparently, this makes it work cleanly with check+handle.

The second example uses more "classic Go" approach, where if an error is passed, an extra message with some context and description is prepended to it. The message tends to be different on each line, so that detailed information about the current phase of the operation can be described with appropriate wording. Surprisingly, this "classic Go" approach seems to make the code not work well with check+handle. Trying to add a separate handle block before each newError would seem to explode the line count and increase messiness.

The "detailed message" as a string, a.k.a. "classic Go" approach, has pros and cons. It's nice that it doesn't depend on particular line in source code, so that searching for a message in codebase will usually show correct location even if the code was modified. But it also makes the final error message potentially super long and overdetailed, if the error object passed through a stack of numerous functions. It could be valuable to consider how this could be improved, possibly by using some mechanisms proposed in the "error values" design draft. Maybe the check and/or handle keyword could be extended, so that a list of "extra details" to be added to the "error value" could be listed by user?

Also, I had a thought regarding stack traces and their performance impact. What if the default handle block would add just the information (PC) about only the current stack frame, as an "error detail"? In such case, the building of the stack trace would be potentially amortized over the whole call stack. Early capture and discarding of the error could maybe reduce the overhead in such case, by limiting the number of analyzed stack frames. One disadvantage of this approach, is that this would make the default handle block quite complex, which doesn't feel very Go-like to me.

@pborman

This comment has been minimized.

Copy link

commented Aug 29, 2018

In your original code, if you name your return value, after line 8, you can have:

defer func() {
    if err != nil {
        t.hardReset()
    }
}()

At this point you don't need a handle at all. Named return values and defer are very powerful. I then see check as a way to eliminate , err and if err != nil { return err } from the code, and nothing more.

@elimisteve

This comment has been minimized.

Copy link

commented Sep 8, 2018

@pborman Excellent point

@akavel

This comment has been minimized.

Copy link
Owner Author

commented Sep 13, 2018

Sorry for a late reply, github didn't seem to notify me of comments here! (Or did I just overlook them? :/ it's first time I'm publishing a gist like this...)

@pborman Interesting, thanks! (I tend to avoid named return values, at least at my workplace we kinda have a policy against them for now, as we felt they're somewhat tricky. Now I see this results in me having little experience with them and what they enable.) Does it mean that handle err is essentially equivalent to the pattern you show — with one crucial exception being that handle is lexically scoped, while defer is function scoped?

@akavel

This comment has been minimized.

Copy link
Owner Author

commented Oct 31, 2018

A thought experiment, with a tentative working syntax // assert: (it is in comment and in column 1 to make it fade out with current syntax highlighter on GitHub). I've been having thoughts on "how to make code show more of the algorithm essence" for some time, this is one current exploration of what I've found interesting to ponder upon:

func (b *Bus) readAndUnpack() ([]byte, error) {
	n, err := io.ReadAtLeast(b.port, b.buf[:], 2)
	got := b.buf[:n]
// assert: err == nil, newError("xxx: cannot read 2-byte preamble [got: % 02X] - error: %s", got, err)
// assert: got[0] == magic_number, newError("xxx: bad MAGIC NUMBER in response - message starts with: [% 02X] (expected XX...)", got)
	length := int(got[1])
// assert: length >= 5, newError("xxx: response too short: len=%d < 5 [% 02X]", length, got)

	// Now that we know the total length, we can read the remaining bytes of the response
	if n < length {
		n, err = io.ReadAtLeast(b.port, b.buf[n:], length-n)
// assert: err == nil, newError("xxx: cannot read remaining bytes of a packet [prefix=% 02X][rest=% 02X] - error: %s", got, b.buf[len(got):][:n], err)
		got = b.buf[:len(got)+n]
	}
	crc := crc(got[:length-2])
// assert: crc == [2]byte{got[length-2], got[length-1]}, newError("xxx: bad CRC [% 02X], expected [...% 02X]", got, crc[:])
	payload := append([]byte(nil), got[2:length-2]...)
	return payload, nil
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.