Skip to content

Commit

Permalink
fix: errors when deleting process track groups
Browse files Browse the repository at this point in the history
- employ a similar strategy as in the Track to cache the last known
  state in case it has been deleted. This ensures that continued
  clicking on the delete button before the track group is unrendered
  will not throw up assertion errors
- let the track group toggle action be tolerant of non-existent track
  groups. This ensures that clicking on the title area of a track
  group before it is unredered does not throw up assertion errors
- add missing "if exists" clauses to drop statements. These ensure,
  especially for the case of deleting a track group for a process that
  has a summary track, that the "no such table/view" errors are not
  thrown when deleting a track group

Signed-off-by: Christian W. Damus <[email protected]>
  • Loading branch information
cdamus committed Sep 7, 2023
1 parent 4558447 commit a291266
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 6 deletions.
8 changes: 6 additions & 2 deletions ui/src/common/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -611,8 +611,12 @@ export const StateActions = {
toggleTrackGroupCollapsed(state: StateDraft, args: {trackGroupId: string}):
void {
const id = args.trackGroupId;
const trackGroup = assertExists(state.trackGroups[id]);
trackGroup.collapsed = !trackGroup.collapsed;
const trackGroup = state.trackGroups[id];
// If the track group was deleted in the interim, then
// we've nothing to collapse and that's fine
if (trackGroup) {
trackGroup.collapsed = !trackGroup.collapsed;
}
},

requestTrackReload(state: StateDraft, _: {}) {
Expand Down
2 changes: 1 addition & 1 deletion ui/src/controller/flamegraph_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class TablesCache {
// TODO(hjd): This should be LRU.
if (this.cache.size > this.cacheSizeLimit) {
for (const name of this.cache.values()) {
await this.engine.query(`drop table ${name}`);
await this.engine.query(`drop table if exists ${name}`);
}
this.cache.clear();
}
Expand Down
21 changes: 20 additions & 1 deletion ui/src/frontend/track_group_panel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ export class TrackGroupPanel extends Panel<Attrs> {
private backgroundColor = '#ffffff'; // Updated from CSS later.
private summaryTrack: Track|undefined;

// Caches the last state.trackGroups[this.trackGroupId].
// This is to deal with track group deletion. See comments
// in trackGroupState() below.
private lastTrackGroupState: TrackGroupState;

constructor({attrs}: m.CVnode<Attrs>) {
super();
this.trackGroupId = attrs.trackGroupId;
Expand All @@ -61,10 +66,24 @@ export class TrackGroupPanel extends Panel<Attrs> {
this.summaryTrack =
trackCreator.create({trackId: this.summaryTrackState.id, engine});
}
this.lastTrackGroupState = assertExists(
globals.state.trackGroups[this.trackGroupId]);
}

get trackGroupState(): TrackGroupState {
return assertExists(globals.state.trackGroups[this.trackGroupId]);
// We can end up in a state where a Track Group is still in the mithril
// renderer tree but its corresponding state has been deleted. This can
// happen in the interval of time between a group being removed from the
// state and the next animation frame that would remove the group object.
// If a mouse event is dispatched in the meanwhile (or a promise is
// resolved), we need to be able to access the state. Hence the caching
// logic here.
const result = globals.state.trackGroups[this.trackGroupId];
if (result === undefined) {
return this.lastTrackGroupState;
}
this.lastTrackGroupState = result;
return result;
}

get summaryTrackState(): TrackState {
Expand Down
4 changes: 2 additions & 2 deletions ui/src/tracks/process_summary/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,8 @@ class ProcessSummaryTrackController extends TrackController<Config, Data> {

onDestroy(): void {
if (this.setup) {
this.query(`drop table ${this.tableName('window')}`);
this.query(`drop table ${this.tableName('span')}`);
this.query(`drop table if exists ${this.tableName('window')}`);
this.query(`drop table if exists ${this.tableName('span')}`);
this.setup = false;
}
}
Expand Down

0 comments on commit a291266

Please sign in to comment.