Fix panic in temporal functions; check err before deferring. #1429

andrewmains12 · 2019-03-04T21:10:03Z

Fix for #1428 .

I've added tests that:

reproduce the issue on master
make sure that we're still closing everything.

The tests are a bit wonky due to some 2 level mocking I needed to do--let me know if you have questions or suggestions for improvements there!

src/query/generated/mocks/generate.go

andrewmains12 · 2019-03-04T21:17:57Z

.excludelint

@@ -1,4 +1,5 @@
 (vendor/)
 (generated/)
 (_mock.go)
+(_mock_test.go)


This btw, is to allow us to generate private mocks purely within testing scope; I don't want to export some mocks.

benraskin92 · 2019-03-04T21:30:14Z

src/query/functions/temporal/base.go

 	cache         *blockCache
 	processor     Processor
 	transformOpts transform.Options
 }

+// controller is a minimal interface of transform.Controller, used exclusively


Not a huge fan of having test specific stuff in our code. Anyway we can do this without putting this here?

Agreed--let me explain what I'm trying to do; maybe there's a good way to do it without this :)

I want to test that blocks we create are closed (especially in case of an error). That means that I need a block that allows me to make that assertion; regular blocks don't (which is reasonable). Since we create blocks via controller.NewBlockBuilder, controller is my entry point into injecting a new block. Therefore, I mock out controller.

Alternatives considered:

create a public transform.Controller interface, and use that everywhere instead. I didn't think it was worth refactoring every usage of controller for this, but we could.

I'm definitely in favour of the alternative here; if I'm not mistaken the refactor should be pretty low-impact (mostly changing *transform.Controller to transform.Controller)

So it's not too bad, but given that this is a bug fix for a panic, I'd rather get this in first and do the refactor after. Thoughts?

If we're going more hotfix then I'd say jam it with no test; if it's not a hotfix wouldn't mind having the refactor in, but will be happy enough with pulling this into an isolated file then filing an explicit issue if that sounds good?

Sure thing--filed #1430 to track. Will do that shortly!

codecov · 2019-03-04T21:32:38Z

Codecov Report

Merging #1429 into master will increase coverage by <.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #1429     +/-   ##
========================================
+ Coverage    70.8%   70.8%   +<.1%     
========================================
  Files         834     834             
  Lines       71487   71488      +1     
========================================
+ Hits        50645   50673     +28     
+ Misses      17534   17514     -20     
+ Partials     3308    3301      -7

Flag	Coverage Δ
#aggregator	`82.3% <ø> (ø)`	⬆️
#cluster	`85.8% <ø> (ø)`	⬆️
#collector	`63.7% <ø> (ø)`	⬆️
#dbnode	`80.8% <ø> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.2% <ø> (-0.1%)`	⬇️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (ø)`	⬆️
#query	`65.7% <100%> (ø)`	⬆️
#x	`76% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d78eee5...134a96b. Read the comment docs.

arnikola · 2019-03-04T21:12:51Z

src/query/functions/temporal/base.go

@@ -256,15 +267,16 @@ func (c *baseNode) processCompletedBlocks(queryCtx *models.QueryContext, process
 	sp, _ := opentracingutil.StartSpanFromContext(queryCtx.Ctx, c.op.OpType())
 	defer sp.Finish()

-	blocks := make([]block.Block, len(processRequests))
-	for i, req := range processRequests {
+	blocks := make([]block.Block, 0, len(processRequests))


Why this change?

Ah yeah this is the reason for the panic. Previously, I was allocating blocks with length = len(processRequests), which meant that the array was:

[]block.Block{nil, nil, ...}

In case of early return (on error), we'd loop through every block and try to close it, causing a segfault.

Ah gotcha; would it be worth adding nil checks into closeBlocks?

Eh I don't think so; there's no valid use case for having a nil element in the slice. That probably means I should a check in the test though--good call!

arnikola · 2019-03-05T18:15:13Z

src/query/functions/temporal/base_test.go

+	BuiltBlock *closeSpyBlock
+}
+
+func (bbc *closeSpyBlockBuilder) Build() block.Block {


Maybe I've spent too long on the internet, but could we rename this to (b *closeSpyBlockBuilder)?

Haha yeah good call.

arnikola · 2019-03-05T18:15:32Z

src/query/functions/temporal/base_test.go

+	Closed bool
+}
+
+func (bwc *closeSpyBlock) Close() error {


Same here, can we rename this to (b *closeSpyBlock)

src/query/functions/temporal/base_test.go

arnikola · 2019-03-05T18:17:04Z

src/query/functions/temporal/base_test.go

+type closeSpyBlock struct {
+	block.Block
+
+	Closed bool


This can probably be private

So I don't know that style guides cover this, but my general rule is to make member's public when they're meant to be read outside of the struct, as this one is. That applies even if the class itself is private to the package.

Private struct fields imo are more for internal state where nothing outside of the struct should read the fields, which isn't true here.

Don't have very strong thoughts on it myself; personally prefer to default to making everything as closed as possible, then open it when necessary

For sure! It's worth noting that both options are actually equivalent in terms of visibility; in both cases, everything in the package can see the field, and nothing outside the package can. That's why I tend to treat it as a semantic marker.

Anyhow, just a point of discussion :)

True enough; I guess there's a little weirdness in public accessors on private fields but that seems like a general go problem

arnikola · 2019-03-05T18:23:09Z

src/query/functions/temporal/base.go

 	cache         *blockCache
 	processor     Processor
 	transformOpts transform.Options
 }

+// controller is a minimal interface of transform.Controller, used exclusively


I'm definitely in favour of the alternative here; if I'm not mistaken the refactor should be pretty low-impact (mostly changing *transform.Controller to transform.Controller)

arnikola · 2019-03-05T18:25:20Z

src/query/functions/temporal/base_test.go

@@ -351,6 +353,158 @@ func setup(numBlocks int, duration time.Duration, nextBound int) *testContext {
 	}
 }

+// TestBaseWithDownstreamError checks that we handle errors from blocks correctly
+func TestBaseWithDownstreamError(t *testing.T) {


Don't know if you have the time for it, but this would be a good test to run on other functions as well; if you don't want to do it now maybe just put up an issue for it linking to this subject?

but this would be a good test to run on other functions as well

Probably, but I'd rather fix the panic first :). I can put up a separate issue for it.

Sounds good 👍

arnikola · 2019-03-05T18:25:32Z

src/query/functions/temporal/base_test.go

+// an UnconsolidatedBlock which errors on SeriesIter() (unconsolidatedBlockWithSeriesIterErr)
+type blockWithDownstreamErr struct {
+	block.Block
+	Err error


Can probably be private

See above response

arnikola · 2019-03-05T18:27:26Z

src/query/functions/temporal/base_test.go

+
+type unconsolidatedBlockWithSeriesIterErr struct {
+	block.UnconsolidatedBlock
+	Err error


Can probably be private

See above response

arnikola · 2019-03-05T18:28:36Z

src/query/functions/temporal/base_test.go

+	require.EqualError(t, err, testErr.Error())
+}
+
+// Types for TestBaseWithDownstreamError


nit: I think may be a bit clearer to group a bunch of these test types together? Up to you

Personally I like them closer to where they're used, since they're effectively part of the particular tests (I'd put them inside the tests if I could, but alas, no method definitions within functions :/ )

Yeah totally fair; I guess the tradeoff is extra clarity for particular tests when the utils are defined next to them or extra clarity for what the tests themselves cover when they're grouped

Either way super minor nitpick haha

arnikola

Approved with a final comment

andrewmains12 requested review from arnikola and benraskin92 March 4, 2019 21:10

andrewmains12 commented Mar 4, 2019

View reviewed changes

src/query/generated/mocks/generate.go Outdated Show resolved Hide resolved

andrewmains12 force-pushed the andrewmains12/fix_temporal_panic branch from 09413d2 to 578aacd Compare March 4, 2019 21:17

andrewmains12 commented Mar 4, 2019

View reviewed changes

benraskin92 reviewed Mar 4, 2019

View reviewed changes

andrewmains12 force-pushed the andrewmains12/fix_temporal_panic branch from 578aacd to 1af927d Compare March 4, 2019 21:30

andrewmains12 force-pushed the andrewmains12/fix_temporal_panic branch 5 times, most recently from 2c99d15 to f5fa1c9 Compare March 5, 2019 15:48

arnikola reviewed Mar 5, 2019

View reviewed changes

andrewmains12 force-pushed the andrewmains12/fix_temporal_panic branch 2 times, most recently from 1bb49a5 to a245566 Compare March 5, 2019 21:30

arnikola approved these changes Mar 6, 2019

View reviewed changes

andrewmains12 force-pushed the andrewmains12/fix_temporal_panic branch 2 times, most recently from f9d2abd to 134a96b Compare March 6, 2019 17:24

andrewmains12 added 4 commits March 6, 2019 13:55

WIP: add test which repros issue, fix issue

e7428ca

More complicated tests for close

50cfa21

Just use regular _mock.go suffix

b15c750

Review comments

632b37d

andrewmains12 force-pushed the andrewmains12/fix_temporal_panic branch from 134a96b to 632b37d Compare March 6, 2019 19:17

andrewmains12 merged commit 2454a92 into master Mar 6, 2019

andrewmains12 deleted the andrewmains12/fix_temporal_panic branch March 6, 2019 19:36

arnikola mentioned this pull request Mar 12, 2019

Complete changelog for 0.7.0 #1443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic in temporal functions; check err before deferring. #1429

Fix panic in temporal functions; check err before deferring. #1429

andrewmains12 commented Mar 4, 2019

andrewmains12 Mar 4, 2019

benraskin92 Mar 4, 2019

andrewmains12 Mar 4, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

codecov bot commented Mar 4, 2019 •

edited

Loading

arnikola Mar 4, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 6, 2019

andrewmains12 Mar 6, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 5, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 5, 2019

arnikola Mar 5, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 5, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 5, 2019

andrewmains12 Mar 5, 2019

arnikola Mar 5, 2019

arnikola left a comment

Fix panic in temporal functions; check err before deferring. #1429

Fix panic in temporal functions; check err before deferring. #1429

Conversation

andrewmains12 commented Mar 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2019 •

edited

Loading