panicOnError() does not behave properly with context cancellation #59
Comments
|
From @AWoloszyn |
|
@silence-do-good this has been open for a while, are you able to work on it? |
I haven't been able to get this reproduced on the System Profiler side yet. Was having this bug open in case it occurs, but we can close it for now and reopen if it gets reported again. |
|
This may not happen in the system profiler but is 100% reproducible when looking at a graphics trace. |
|
Without a good reproduction case, it's difficult to act on it soon. Will keep this open as P2 so we can work on it in the next bug-hunt iteration. |
|
If you click in the UI faster than the server can respond, this will eventually lead to a crash. |
|
To be clear this happens on Vulkan traces. No idea about the systems profiler. |
If the context gets cancelled during capture command mutation,
panicOnError() may be triggered and leads to a GAPIS panic. This
change catches such panics and transform them to regular errors.
Another option would have been to get rid of panicOnError and
propagate the errors across all generated code inside Mutate
functions: I had a quick try at this, and it gets quite
cumbersome, especially things like:
```
foo := bar(..., MustRead(...), ...)
```
must be turned into:
```
tmp, err := MustRead(...)
if err != nil {
return nil, err
}
foo := bar(..., tmp, ...)
```
Using panic for control flow is not awesome, but it is very convenient
here to jump from possibly deep-nested calls in generated code up to
the top-level of Mutate.
Fix google#59
Bug: b/149701723
Test: manual, by hard-coding panics and errors
If the context gets cancelled during capture command mutation,
panicOnError() may be triggered and leads to a GAPIS panic. This
change catches such panics and transform them to regular errors.
Another option would have been to get rid of panicOnError and
propagate the errors across all generated code inside Mutate
functions: I had a quick try at this, and it gets quite
cumbersome, especially things like:
```
foo := bar(..., MustRead(...), ...)
```
must be turned into:
```
tmp, err := MustRead(...)
if err != nil {
return nil, err
}
foo := bar(..., tmp, ...)
```
Using panic for control flow is not awesome, but it is very convenient
here to jump from possibly deep-nested calls in generated code up to
the top-level of Mutate.
Fix google#59
Bug: b/149701723
Test: manual, by hard-coding panics and errors
|
Fixed with #639 |
GAPIS can arbitrarily crash when a context is cancelled, if it happens in some api.go functions.
The text was updated successfully, but these errors were encountered: