Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix for circular references #659

Merged
merged 5 commits into from
Oct 5, 2018
Merged

Fix for circular references #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
21878fe
Fix for circular references
Oct 4, 2018
f193f57
Clean up comments in the test class
Oct 4, 2018
0f57355
Comment removal pt2
Oct 4, 2018
8b64090
change map to set
Oct 5, 2018
89c0699
Merge branch 'develop' into jsuplizio/CircularRefFix
Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 113 additions & 44 deletions packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ export class HeapDumpService {
const updatedVariable = this.createVariableFromReference(
symbolName,
refVar,
new Map<string, null>()
new Map<string, null>(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replaceFrameVariablesWithHeapDump is quite long.
It is worth considering breaking this up into smaller methods, which each method having a more descriptive name than having all the comments.

I don't know if you have time to do this now but, in general, this code type of long method code is not maintainable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can create a work item to do this but it's not happening right now. There are other bugs that need to get immediately looked at from the Blitz.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

new Array<ApexVariableContainer>()
);
if (updatedVariable) {
frameInfo.locals.set(localVar.name, updatedVariable);
Expand Down Expand Up @@ -173,7 +174,8 @@ export class HeapDumpService {
const updatedVariable = this.createVariableFromReference(
symbolName,
refVar,
new Map<string, null>()
new Map<string, null>(),
new Array<ApexVariableContainer>()
);
if (updatedVariable) {
statics.set(staticVar.name, updatedVariable);
Expand Down Expand Up @@ -210,7 +212,8 @@ export class HeapDumpService {
const updatedVariable = this.createVariableFromReference(
symName,
refVar,
new Map<string, null>()
new Map<string, null>(),
new Array<ApexVariableContainer>()
);
if (updatedVariable) {
frameInfo.globals.set(symName, updatedVariable);
Expand Down Expand Up @@ -298,7 +301,8 @@ export class HeapDumpService {
// cloned into a new variable that has a variable name.
private copyReferenceContainer(
refContainer: ApexVariableContainer,
varName: string
varName: string,
createVarRef = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was necessary because the variablesRef (which is what tells VS Code that the variable can be expanded) didn't need to be recreated for a variable that's part of the circular reference. The reason cloning was necessary was due to the name changing. For instance local var MyVar of type Foo has field that's a List and we add MyVar to its own list. The name of the local variable is MyVar but once added to the list the name is '0' or whatever number in the list it is.

) {
const tmpContainer = new ApexVariableContainer(
varName,
Expand All @@ -311,9 +315,15 @@ export class HeapDumpService {
// set so it can be expanded in the variables window. Note this check
// is kind of superfluous but it's better to be safe than sorry
if (!this.isPrimitiveType(tmpContainer.type)) {
tmpContainer.variablesRef = this.logContext
.getVariableHandler()
.create(tmpContainer);
if (createVarRef) {
tmpContainer.variablesRef = this.logContext
.getVariableHandler()
.create(tmpContainer);
} else {
// If the variable is being cloned for a name change then
// it needs to use the same variablesRef as the parent
tmpContainer.variablesRef = refContainer.variablesRef;
}
}
return tmpContainer;
}
Expand All @@ -327,7 +337,8 @@ export class HeapDumpService {
// 1: '2'
// 2: '3'
private createStringFromVarContainer(
varContainer: ApexVariableContainer
varContainer: ApexVariableContainer,
visitedSet: Set<string>
): string {
// If the varContainer isn't a reference or is a string references
if (
Expand All @@ -336,36 +347,55 @@ export class HeapDumpService {
) {
return varContainer.value;
}
// For a list or set the name string is going to end up being (<val1>,...<valX)
// For a objects, the name string is going to end up being <TypeName>: {<Name1>=<Value1>,...<NameX>=<ValueX>}
const isListOrSet =
varContainer.type.toLowerCase().startsWith('list<') ||
varContainer.type.toLowerCase().startsWith('set<');
// The live debugger, for collections, doesn't include their type in variable name/value
const containerType = this.isCollectionType(varContainer.type)
? ''
: `${varContainer.type}:`;
let returnString = isListOrSet ? '(' : `${containerType}{`;
let first = true;
// Loop through each of the container's variables to get the name/value combinations, calling to create
// the string if another collection is found.
for (const entry of Array.from(varContainer.variables.entries())) {
const valueAsApexVar = entry[1] as ApexVariableContainer;
if (!first) {
returnString += ', ';
}
if (valueAsApexVar.ref) {
// if this is also a ref then create the string from that
returnString += this.createStringFromVarContainer(valueAsApexVar);

// If the varContainer is a ref and it's already been visited then return the string
if (varContainer.ref) {
if (visitedSet.has(varContainer.ref)) {
return 'already output';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing that this is the text matches the interactive debugger?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The pictures in the description illustrate live VS replay which is why I'd used "already output"

} else {
// otherwise get the name/value from the variable
returnString += isListOrSet
? valueAsApexVar.value
: `${valueAsApexVar.name}=${valueAsApexVar.value}`;
visitedSet.add(varContainer.ref);
}
}
let returnString = '';
try {
// For a list or set the name string is going to end up being (<val1>,...<valX)
// For a objects, the name string is going to end up being <TypeName>: {<Name1>=<Value1>,...<NameX>=<ValueX>}
const isListOrSet =
varContainer.type.toLowerCase().startsWith('list<') ||
varContainer.type.toLowerCase().startsWith('set<');
// The live debugger, for collections, doesn't include their type in variable name/value
const containerType = this.isCollectionType(varContainer.type)
? ''
: `${varContainer.type}:`;
returnString = isListOrSet ? '(' : `${containerType}{`;
let first = true;
// Loop through each of the container's variables to get the name/value combinations, calling to create
// the string if another collection is found.
for (const entry of Array.from(varContainer.variables.entries())) {
const valueAsApexVar = entry[1] as ApexVariableContainer;
if (!first) {
returnString += ', ';
}
if (valueAsApexVar.ref) {
// if this is also a ref then create the string from that
returnString += this.createStringFromVarContainer(
valueAsApexVar,
visitedSet
);
} else {
// otherwise get the name/value from the variable
returnString += isListOrSet
? valueAsApexVar.value
: `${valueAsApexVar.name}=${valueAsApexVar.value}`;
}
first = false;
}
returnString += isListOrSet ? ')' : '}';
} finally {
if (varContainer.ref) {
visitedSet.delete(varContainer.ref);
}
first = false;
}
returnString += isListOrSet ? ')' : '}';
return returnString;
}

Expand Down Expand Up @@ -577,7 +607,10 @@ export class HeapDumpService {
}
// If the reference doesn't contain any child references then it's value can set here.
if (!hasInnerRefs) {
refContainer.value = this.createStringFromVarContainer(refContainer);
refContainer.value = this.createStringFromVarContainer(
refContainer,
new Set<string>()
);
}
}

Expand All @@ -597,24 +630,44 @@ export class HeapDumpService {
public createVariableFromReference(
varName: string,
refVariable: ApexVariableContainer,
visitedMap: Map<string, null>
visitedMap: Map<string, ApexVariableContainer | null>,
updateAfterVarCreation: ApexVariableContainer[]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UpdateAfterVarCreation is used in the finally clause. I ended up getting into a chicken and egg situation here and some variable values cannot be updated until the variable is finished being created. This is only applicable to circular references. createVariableFromReference is a recursive function and because of the visitedMap we when we remove the last entry we know we're done with the initial variable being created. It's at this that time it's safe to update the values of any variables that we couldn't update during creation because we know the variable is complete. Trying to create the value earlier, when the variable was incomplete, would end up creating a value that was also incomplete.

): ApexVariableContainer | undefined {
// if this isn't a reference?
if (!refVariable.ref) {
return undefined;
}
// If this reference has already been seen, return
// If this reference has already been seen, then there's
// a good chance that the variable is still being created
// and we can't reset set the value now.
if (visitedMap.has(refVariable.ref)) {
const visitedVar = visitedMap.get(
refVariable.ref
) as ApexVariableContainer;
if (visitedVar !== null) {
if (visitedVar.name !== varName) {
const updatedNameVarContainer = this.copyReferenceContainer(
visitedVar,
varName,
false
);
updateAfterVarCreation.push(updatedNameVarContainer);
return updatedNameVarContainer;
} else {
return visitedVar;
}
}
return undefined;
}
try {
visitedMap.set(refVariable.ref, null);

// First, clone the reference into the named variable
const namedVarContainer = this.copyReferenceContainer(
refVariable,
varName
);
// Create the visitedMap entry with what will be the actual
// variable.
visitedMap.set(refVariable.ref, namedVarContainer);

// If the value hasn't been set yet, then we have to walk through all of the children and update
// any child references on the variable with a recursive call
Expand All @@ -641,7 +694,8 @@ export class HeapDumpService {
const updatedKeyVarContainer = this.createVariableFromReference(
KEY_VALUE_PAIR_KEY,
keyRef!,
visitedMap
visitedMap,
updateAfterVarCreation
);
if (updatedKeyVarContainer) {
keyName = updatedKeyVarContainer.value;
Expand Down Expand Up @@ -670,7 +724,8 @@ export class HeapDumpService {
const updatedValueVarContainer = this.createVariableFromReference(
KEY_VALUE_PAIR_VALUE,
valueRef!,
visitedMap
visitedMap,
updateAfterVarCreation
);
if (updatedValueVarContainer) {
valueVal = updatedValueVarContainer.value;
Expand Down Expand Up @@ -710,7 +765,8 @@ export class HeapDumpService {
const updatedChildContainer = this.createVariableFromReference(
childVarName,
childVarRefContainer!,
visitedMap
visitedMap,
updateAfterVarCreation
);
// update the child variable in the map
if (updatedChildContainer) {
Expand All @@ -727,14 +783,27 @@ export class HeapDumpService {
}
}
namedVarContainer.value = this.createStringFromVarContainer(
namedVarContainer
namedVarContainer,
new Set<string>()
);
}

return namedVarContainer;
} finally {
// Ensure the current reference is removed from the visited map
visitedMap.delete(refVariable.ref);
// If the visited map is empty that means the variable is done being hooked up
// If there are any items in the updateAfterVarCreation array then now is the
// time to update them
if (visitedMap.size === 0) {
updateAfterVarCreation.forEach(element => {
const varContainer = element as ApexVariableContainer;
varContainer.value = this.createStringFromVarContainer(
varContainer,
new Set<string>()
);
});
}
}
}

Expand Down
Loading