Skip to content

Clone object proxies in mapper/resource params #519

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

Merged
merged 1 commit into from
Nov 22, 2024
Merged

Clone object proxies in mapper/resource params #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion skiplang/skjson/ts/src/skjson.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,9 @@ type ObjectProxy<Base extends { [k: string]: Exportable }> = {
keys: IterableIterator<keyof Base>;
} & Base;

function isObjectProxy(x: any): x is ObjectProxy<{ [k: string]: Exportable }> {
export function isObjectProxy(
x: any,
): x is ObjectProxy<{ [k: string]: Exportable }> {
return sk_isObjectProxy in x && (x[sk_isObjectProxy] as boolean);
}

Expand Down
33 changes: 16 additions & 17 deletions skipruntime-ts/wasm/src/internals/skipruntime_module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import { NonUniqueValueException } from "@skipruntime/api";
import { Frozen, type Constant } from "@skipruntime/api/internals.js";

import type { Exportable, SKJSON } from "@skip-wasm/json";
import { isObjectProxy } from "@skip-wasm/json";
import { UnknownCollectionError } from "@skipruntime/helpers/errors.js";

export type Handle<T> = Internal.Opaque<int, { handle_for: T }>;
Expand Down Expand Up @@ -64,22 +65,20 @@ abstract class SkFrozen extends Frozen {
}
}

export function check<T>(value: T): void {
function checkOrCloneParam<T>(value: T): T {
if (
typeof value == "string" ||
typeof value == "number" ||
typeof value == "boolean"
) {
return;
} else if (typeof value == "object") {
if (value === null || isSkFrozen(value)) {
return;
} else {
throw new Error("Invalid object: must be deep-frozen.");
}
} else {
throw new Error(`'${typeof value}' cannot be deep-frozen.`);
)
return value;
if (typeof value == "object") {
if (value === null) return value;
if (isObjectProxy(value)) return value.clone() as T;
Copy link
Contributor

@jberdine jberdine Nov 22, 2024

Choose a reason for hiding this comment

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

Why call clone here instead of in deepFreeze? Do we need a distinct clone for each time it gets checked? If it needs to be here, then check is confusingly misnamed.

Copy link
Contributor Author

@bennostein bennostein Nov 22, 2024

Choose a reason for hiding this comment

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

My understanding (@skiplabsdaniel is the authority) is that this is a distinct issue from deep-freezing: these proxy objects are frozen, but when they're used as parameters to mappers and resources, their skip-heap pointers can outlive the lifetime of the objects they point to. So it's not always necessary to clone when freezing: only at this point when the proxy is used as a parameter.

Agreed on the naming point, will change.

Copy link
Contributor

@jberdine jberdine Nov 22, 2024

Choose a reason for hiding this comment

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

Ok, but I don't understand. :-) Is it a pointer stored in the skip heap to a proxy object in the js heap that is the problem? Is the proxy in the js heap really just supposed to be a wrapper for a skip object? So would it make sense to have the skip heap hold a pointer the the proxied skip object instead?

It isn't clear how calling clone here will make a difference to the relative lifetimes. I worry that this is a bug waiting to happen since the clone call looks like a nop and someone could easily remove it later.

if (isSkFrozen(value)) return value;
throw new Error("Invalid object: must be deep-frozen.");
}
throw new Error(`'${typeof value}' cannot be deep-frozen.`);
}

/**
Expand Down Expand Up @@ -985,8 +984,8 @@ class EagerCollectionImpl<K extends Json, V extends Json>
mapper: new (...params: Params) => Mapper<K, V, K2, V2>,
...params: Params
): EagerCollection<K2, V2> {
params.forEach(check);
const mapperObj = new mapper(...params);
const mapperParams = params.map(checkOrCloneParam) as Params;
const mapperObj = new mapper(...mapperParams);
Object.freeze(mapperObj);
if (!mapperObj.constructor.name) {
throw new Error("Mapper classes must be defined at top-level.");
Expand All @@ -1011,8 +1010,8 @@ class EagerCollectionImpl<K extends Json, V extends Json>
reducer: Reducer<V2, V3>,
...params: Params
) {
params.forEach(check);
const mapperObj = new mapper(...params);
const mapperParams = params.map(checkOrCloneParam) as Params;
const mapperObj = new mapper(...mapperParams);
Object.freeze(mapperObj);
if (!mapperObj.constructor.name) {
throw new Error("Mapper classes must be defined at top-level.");
Expand Down Expand Up @@ -1116,8 +1115,8 @@ class ContextImpl extends SkFrozen implements Context {
compute: new (...params: Params) => LazyCompute<K, V>,
...params: Params
): LazyCollection<K, V> {
params.forEach(check);
const computeObj = new compute(...params);
const mapperParams = params.map(checkOrCloneParam) as Params;
const computeObj = new compute(...mapperParams);
Object.freeze(computeObj);
if (!computeObj.constructor.name) {
throw new Error("LazyCompute classes must be defined at top-level.");
Expand Down