-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
"panic: runtime error: index out of range" from spread attempt on error value #4349
Comments
A record spread expression that produces an empty record (e.g., "{...a}" evaluated with "this" bound to a Zed primitive value) results in a zed.Value with malformed bytes, which can cause a panic elsewhere. Fix this in recordSpreadExpr.Eval by returning a well-formed zed.Value. For better or worse, we don't have any existing tests for malfored zed.Values, so I'm not adding one here. Closes #4349.
A record spread expression that produces an empty record (e.g., "{...a}" evaluated with "this" bound to a Zed primitive value) results in a zed.Value with malformed bytes, which can cause a panic elsewhere. Fix this in recordSpreadExpr.Eval by returning a well-formed zed.Value. For better or worse, we don't have any existing tests for malformed zed.Values, so I'm not adding one here. Closes #4349.
A record spread expression that produces an empty record (e.g., "{...a}" evaluated with "this" bound to a Zed primitive value) results in a zed.Value with malformed bytes, which can cause a panic elsewhere. Fix this in recordSpreadExpr.Eval by returning a well-formed zed.Value. For better or worse, we don't have any existing tests for malformed zed.Values, so I'm not adding one here. Closes #4349.
Verified in Zui commit As shown in the attached video, the spread attempt no longer causes a panic and stack dump in the Zed lake log. Verify.mp4Also here's proof of success with the CLI tooling:
Alas, the video shows the app still struggling to correctly present the value, so I've added a comment to an existing issue at brimdata/zui#2454 (comment) where a similar problem is already being tracked. Also, I'm still concerned about the fact the panic was not making its way back in the API response to be surfaced to the user, so I've opened a new issue #4363 to track that in general. Thanks @nwt! |
Repro is with Zui commit
b9561cf
which uses Zed da55f70. The repro in Zui is shown for effect, but the problem is ultimately at the Zed layer.I think the community zync user originally triggered this issue because their data had a field
value
that typically stored a record but sometimes it contained an error. Therefore a super simple way I've found to trigger the problem is with just the input data:As shown in the attached video, if this is imported into the app and the query
yield {...value}
is executed, a panic dump appears in the Zed lake log. Thezed serve
process does not exit.Repro.mp4
Reproducing this with
zq
outside the app, the dump is easier to see.While the user's error values seemed like an important ingredient in their originally hitting the bug, I've also found that I can trigger a similar dump with other data such as an empty record and other formats such as CSV.
It also looks like this issue has maybe always been there. He's a repro back at GA Zed tagged
v1.1.0
.In addition the specific crash, I'm also a little troubled by the silence in the app. I used Wireshark to sniff the API traffic and as shown below, the dump we saw in the lake log file doesn't show up in the API response, so it makes sense that we saw nothing in the app.
I'm having difficulty keeping track of what we should expect here regarding such errors. When I last visited the topic in the context of brimdata/zui#2620, unlike what we saw above, the panic then was presented in the app.
In any case, it seems it would be ideal if errors showed up in the log file and the app whenever possible since silence in response to a query is typically interpreted by the user as "no results found" rather than "something crashed". We could open up a separate issue if this topic needs to be handled separately.
The text was updated successfully, but these errors were encountered: