Fix updates failing during output of a notebook #144868

rchiodo · 2022-03-11T01:05:27Z

This PR fixes microsoft/vscode-jupyter#9327

Outputs that contain any data would crash during an update preventing the cell output update from coming through to an extension.

I believe the issue was originally caused by this commit (our tests started failing this morning):
#144679

rchiodo · 2022-03-11T01:06:35Z

src/vs/workbench/api/common/extHostNotebookDocuments.ts

@@ -32,7 +60,7 @@ export class ExtHostNotebookDocuments implements extHostProtocol.ExtHostNotebook
 	$acceptModelChanged(uri: UriComponents, event: SerializableObjectWithBuffers<extHostProtocol.NotebookCellsChangedEventDto>, isDirty: boolean, newMetadata?: NotebookDocumentMetadata): void {
 		const document = this._notebooksAndEditors.getNotebookDocument(URI.revive(uri));
 		const e = document.acceptModelChanged(event.value, isDirty, newMetadata);
-		this._onDidChangeNotebookDocument.fire(deepFreeze(e));
+		this._onDidChangeNotebookDocument.fire(partialFreeze(e));


I was getting this with a full freeze:

ERR Cannot freeze array buffer views with elements: TypeError: Cannot freeze array buffer views with elements at Function.freeze (<anonymous>) at deepFreeze (d:\Source\vscode\out\vs\base\common\objects.js:36:20) at ExtHostNotebookDocuments.$acceptModelChanged (d:\Source\vscode\out\vs\workbench\api\common\extHostNotebookDocuments.js:32:77) at RPCProtocol._doInvokeHandler (d:\Source\vscode\out\vs\workbench\services\extensions\common\rpcProtocol.js:399:27) at RPCProtocol._invokeHandler (d:\Source\vscode\out\vs\workbench\services\extensions\common\rpcProtocol.js:384:45) at RPCProtocol._receiveRequest (d:\Source\vscode\out\vs\workbench\services\extensions\common\rpcProtocol.js:311:32) at RPCProtocol._receiveOneMessage (d:\Source\vscode\out\vs\workbench\services\extensions\common\rpcProtocol.js:243:26) at d:\Source\vscode\out\vs\workbench\services\extensions\common\rpcProtocol.js:115:52 at Listener.invoke (d:\Source\vscode\out\vs\base\common\event.js:420:27) at Emitter.fire (d:\Source\vscode\out\vs\base\common\event.js:559:34)** **

because the output.items.data were not freezable.

rchiodo · 2022-03-11T01:06:59Z

src/vs/workbench/api/common/extHostNotebookDocument.ts

 			if (!append) {
-				output.items.length = 0;
+				output.items.length = 0; // This crashes without a copy


I also noticed this throwing an exception while running the same code.

You have a stack trace? Is it throwing because the event processing froze it? That wouldn't be good...

Yes I believe it's crashing because of the freeze.

rchiodo · 2022-03-11T01:08:33Z

To repro this issue:

Open a jupyter notebook
Put this code into it:

print("Start")
import time
for i in range(100):
  time.sleep(0.1)
  print(i)

print("End")

Exception should be thrown inside of VS code

rchiodo · 2022-03-11T01:10:15Z

src/vs/workbench/api/common/extHostNotebookDocuments.ts

+
+const hasBuffer = (typeof Buffer !== 'undefined');
+
+export function partialFreeze<T>(obj: T): T {


This is a bit messy, hoping you guys have a cleaner way to do this. Maybe modifying the original deepFreeze to do the same?

Maybe modifying the original deepFreeze to do the same?

💯% All users of deepFreeze should benefit from this goodness. There is already a check to decide to decent on an object or not. This check should be extended.

Also, this shouldn't be checking for Buffer - that's a nodejs only type and where are here in common-land. I also believe the error is coming from the underlying typed array, this fails the same way

> var a = new Uint8Array(12) undefined > Object.freeze(a) Uncaught TypeError: Cannot freeze array buffer views with elements at Function.freeze (<anonymous>)

There should be a check iff an object is a typed array - quite a few candidates according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Typed_arrays#typed_array_views. Good news is they are instanceof-able, e.g candidate instanceof Float32Array works etc.

src/vs/workbench/api/common/extHostNotebookDocument.ts

jrieken

Thanks for far. I have added a few comments and suggestions for improvements. I will also push a change that removes the deepFreeze to unblock you.

jrieken · 2022-03-11T08:16:53Z

src/vs/workbench/api/common/extHostNotebookDocuments.ts

+
+const hasBuffer = (typeof Buffer !== 'undefined');
+
+export function partialFreeze<T>(obj: T): T {


Maybe modifying the original deepFreeze to do the same?

💯% All users of deepFreeze should benefit from this goodness. There is already a check to decide to decent on an object or not. This check should be extended.

Also, this shouldn't be checking for Buffer - that's a nodejs only type and where are here in common-land. I also believe the error is coming from the underlying typed array, this fails the same way

> var a = new Uint8Array(12) undefined > Object.freeze(a) Uncaught TypeError: Cannot freeze array buffer views with elements at Function.freeze (<anonymous>)

There should be a check iff an object is a typed array - quite a few candidates according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Typed_arrays#typed_array_views. Good news is they are instanceof-able, e.g candidate instanceof Float32Array works etc.

jrieken · 2022-03-11T08:17:44Z

src/vs/workbench/api/common/extHostNotebookDocument.ts

 			if (!append) {
-				output.items.length = 0;
+				output.items.length = 0; // This crashes without a copy


You have a stack trace? Is it throwing because the event processing froze it? That wouldn't be good...

…rosoft/vscode-jupyter#9327

…_update

src/vs/base/common/types.ts

jrieken

I suggest to take in the changes for deepFreeze (checking for typed array) but no cloning of outputs or such. Instead, the event itself shouldn't likely be using deepFreeze but some level of shallow freeze.

src/vs/workbench/api/common/extHostNotebookDocument.ts

rchiodo · 2022-03-14T16:57:41Z

I suggest to take in the changes for deepFreeze (checking for typed array) but no cloning of outputs or such. Instead, the event itself shouldn't likely be using deepFreeze but some level of shallow freeze.

I modified deepFreeze to support an exclusion list. I then used this at the event location to exclude freezing the outputs.

src/vs/workbench/api/common/extHostNotebookDocument.ts

jrieken · 2022-03-16T10:59:07Z

I modified deepFreeze to support an exclusion list. I then used this at the event location to exclude freezing the outputs.

Not a fan of that tbh - deepFreeze is "deep" and a flat list of key is always ambiguous in that context. Maybe my earlier suggestion was misleading. I like two things from this PR

improve deepFreeze to always ignore typed arrays because it is apparently illegal to freeze them.
don't use deepFreeze when emitting the notebook change event.

It is my bad that I started with deepFreeze here but I came to believe it is wrong to freeze objects that you "don't own", e.g the function that applies the change and emits the event should only shallow freeze the parts of the event it owns, like the event itself, cellChanges, contentChanges etc but not the notebook document or its cells. It's the responsibility of themselves to protect them from outside modifications (which may or may not be the case but that would be a different issue)

…44868

rchiodo · 2022-03-16T16:40:41Z

it is my bad that I started with deepFreeze here but I came to believe it is wrong to freeze objects that you "don't own", e.g the function that applies the change and emits the event should only shallow freeze the parts of the event it owns, like the event itself, cellChanges, contentChanges etc but not the notebook document or its cells. It's the responsibility of themselves to protect them from outside modifications (which may or may not be the case but that would be a different issue)

Yeah that makes more sense. Only freeze event properties, not the notebook itself.

jrieken · 2022-03-18T14:45:46Z

src/vs/workbench/api/common/extHostNotebookDocument.ts

@@ -294,6 +294,10 @@ export class ExtHostNotebookDocument {
 			}
 		}

+		// Freeze event properties so handlers cannot accidentally modify them
+		deepFreeze(result.cellChanges);
+		deepFreeze(result.contentChanges);


shallow freeze only, otherwise NotebookCell instances are frozen

Yes this should just be object.freeze instead of deepFreeze

Although it's weird that the output wasn't frozen? I think I need to debug the actual contents here to see what's in them. Object. freeze would certainly work but there might be more that can be frozen.

jrieken

lgtm

…868 (comment)

jrieken · 2022-03-21T14:14:23Z

Ops, post merge (while writing the TPI) I realised this was still freezing the notebook itself and its metadata. Pushed another change on top of things

Fix updates failing during output of a notebook

ae5a0a1

rchiodo commented Mar 11, 2022

View reviewed changes

rchiodo requested review from jrieken, mjbvz, roblourens and rebornix March 11, 2022 01:08

rchiodo commented Mar 11, 2022

View reviewed changes

vscode-triage-bot assigned rchiodo Mar 11, 2022

mjbvz reviewed Mar 11, 2022

View reviewed changes

src/vs/workbench/api/common/extHostNotebookDocument.ts Outdated Show resolved Hide resolved

jrieken requested changes Mar 11, 2022

View reviewed changes

jrieken added a commit that referenced this pull request Mar 11, 2022

for now, don't deepFreeze the onDidChangeNotebook event, #144868, mic…

1799e12

…rosoft/vscode-jupyter#9327

rchiodo added 3 commits March 11, 2022 09:10

Merge remote-tracking branch 'upstream/main' into rchiodo/celloutputs…

5ec68e1

…_update

Update deepFreeze to handle freezing of Buffers

046d2cd

Update comment to indicate why readonly

2f9dc74

rchiodo requested a review from jrieken March 11, 2022 17:37

roblourens reviewed Mar 11, 2022

View reviewed changes

src/vs/base/common/types.ts Outdated Show resolved Hide resolved

Handle other typed arrays too

4ac1fc2

jrieken requested changes Mar 14, 2022

View reviewed changes

src/vs/workbench/api/common/extHostNotebookDocument.ts Outdated Show resolved Hide resolved

Allow deepfreeze to skip elements

cc0d784

Fix build problem

34a7ef1

rchiodo commented Mar 14, 2022

View reviewed changes

src/vs/workbench/api/common/extHostNotebookDocument.ts Outdated Show resolved Hide resolved

Remove extra blank line

7126858

jrieken added a commit that referenced this pull request Mar 16, 2022

more readonly-ness for parts of the NotebookDocumentChangeEvent, #1…

cdb839a

…44868

Only freeze event properties

bd199ef

Linter problem

9b1ce8f

rchiodo requested a review from jrieken March 17, 2022 18:46

jrieken requested changes Mar 18, 2022

View reviewed changes

rchiodo added 2 commits March 18, 2022 13:10

Shallow freeze the event properties

523f5cd

Merge branch 'main' into rchiodo/celloutputs_update

cfe81da

rchiodo requested a review from jrieken March 18, 2022 20:52

jrieken approved these changes Mar 21, 2022

View reviewed changes

jrieken merged commit d59f7fc into microsoft:main Mar 21, 2022

jrieken added a commit that referenced this pull request Mar 21, 2022

even more shallow freeze, remove unused function, assert freeze, #144…

60726d4

…868 (comment)

github-actions bot locked and limited conversation to collaborators May 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix updates failing during output of a notebook #144868

Fix updates failing during output of a notebook #144868

rchiodo commented Mar 11, 2022 •

edited

rchiodo Mar 11, 2022

rchiodo Mar 11, 2022

jrieken Mar 11, 2022

rchiodo Mar 11, 2022

rchiodo commented Mar 11, 2022

rchiodo Mar 11, 2022

jrieken Mar 11, 2022

jrieken left a comment

jrieken Mar 11, 2022

jrieken Mar 11, 2022

jrieken left a comment

rchiodo commented Mar 14, 2022

jrieken commented Mar 16, 2022 •

edited

rchiodo commented Mar 16, 2022

jrieken Mar 18, 2022

rchiodo Mar 18, 2022

rchiodo Mar 18, 2022 •

edited

jrieken left a comment

jrieken commented Mar 21, 2022 •

edited


		const hasBuffer = (typeof Buffer !== 'undefined');

		export function partialFreeze<T>(obj: T): T {

Fix updates failing during output of a notebook #144868

Fix updates failing during output of a notebook #144868

Conversation

rchiodo commented Mar 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo commented Mar 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrieken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrieken left a comment

Choose a reason for hiding this comment

rchiodo commented Mar 14, 2022

jrieken commented Mar 16, 2022 • edited

rchiodo commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo Mar 18, 2022 • edited

Choose a reason for hiding this comment

jrieken left a comment

Choose a reason for hiding this comment

jrieken commented Mar 21, 2022 • edited

rchiodo commented Mar 11, 2022 •

edited

jrieken commented Mar 16, 2022 •

edited

rchiodo Mar 18, 2022 •

edited

jrieken commented Mar 21, 2022 •

edited