8000 Protect against `document == null` in `_doc_detached()` by mattpap · Pull Request #14430 · bokeh/bokeh · GitHub
[go: up one dir, main page]
Skip to content

Protect against document == null in _doc_detached() #14430

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
mattpap merged 4 commits into from
Mar 25, 2025
Merged

Protect against document == null in _doc_detached() #14430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a803b1
Protect against `document == null` in `_doc_detached()`
mattpap Mar 20, 2025
ea88615
Make Document.all_models eventually consistent
mattpap Mar 24, 2025
93aae21
Allow to configure model re-computation behavior
mattpap Mar 25, 2025
2f91161
Update Document's unit tests
mattpap Mar 25, 2025
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
32 changes: 6 additions & 26 deletions bokehjs/src/lib/core/has_props.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,9 +535,9 @@ export abstract class HasProps extends Signalable() implements Equatable, Printa
}
}

for (const [prop, old_value, new_value] of changed) {
if (prop.may_have_refs && this._needs_invalidate(old_value, new_value)) {
document._invalidate_all_models()
for (const [prop, _, new_value] of changed) {
if (prop.may_have_refs) {
document.partially_update_all_models(new_value)
break
}
}
Expand Down Expand Up @@ -634,30 +634,10 @@ export abstract class HasProps extends Signalable() implements Equatable, Printa

detach_document(): void {
// This should only be called by the Document implementation to unset the document field
this._doc_detached()
this.document = null
}

protected _needs_invalidate(old_value: unknown, new_value: unknown): boolean {
const new_refs = new Set<HasProps>()
HasProps._value_record_references(new_value, new_refs, {recursive: false})

const old_refs = new Set<HasProps>()
HasProps._value_record_references(old_value, old_refs, {recursive: false})

for (const new_id of new_refs) {
if (!old_refs.has(new_id)) {
return true
}
}

for (const old_id of old_refs) {
if (!new_refs.has(old_id)) {
return true
}
if (this.document != null) {
this._doc_detached()
this.document = null
}

return false
}

protected _push_changes(changes: [Property, unknown, unknown][], sync: boolean): void {
Expand Down
63 changes: 53 additions & 10 deletions bokehjs/src/lib/document/document.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {default_resolver} from "../base"
import {version as js_version} from "../version"
import {logger} from "../core/logging"
import type {Class} from "core/class"
import type {HasProps} from "core/has_props"
import {HasProps} from "core/has_props"
import type {Property} from "core/properties"
import {ModelResolver} from "core/resolvers"
import type {ModelRep} from "core/serialization"
Expand Down Expand Up @@ -78,6 +78,12 @@ export const documents: Document[] = []

export const DEFAULT_TITLE = "Bokeh Application"

export type DocumentOptions = {
roots?: Iterable<HasProps>
resolver?: ModelResolver
recompute_timeout?: number
}

// This class should match the API of the Python Document class
// as much as possible.
export class Document implements Equatable {
Expand All @@ -101,8 +107,9 @@ export class Document implements Equatable {
protected _interactive_timestamp: number | null
protected _interactive_plot: Model | null
protected _interactive_finalize: (() => void) | null
protected _recompute_timeout: number

constructor(options: {roots?: Iterable<HasProps>, resolver?: ModelResolver} = {}) {
constructor(options: DocumentOptions = {}) {
documents.push(this)
this._init_timestamp = Date.now()
this._resolver = options.resolver ?? new ModelResolver(default_resolver)
Expand All @@ -119,6 +126,7 @@ export class Document implements Equatable {
this._idle_roots = new WeakSet()
this._interactive_timestamp = null
this._interactive_plot = null
this._recompute_timeout = options.recompute_timeout ?? 30_000 /* 30s */
if (options.roots != null) {
this._add_roots(...options.roots)
}
Expand Down Expand Up @@ -242,15 +250,30 @@ export class Document implements Equatable {
}
}

/*protected*/ _invalidate_all_models(): void {
logger.debug("invalidating document models")
// if freeze count is > 0, we'll recompute on unfreeze
if (this._all_models_freeze_count === 0) {
protected _recompute_timer: number | null = null

protected _cancel_recompute_all_models(): void {
if (this._recompute_timer != null) {
clearTimeout(this._recompute_timer)
this._recompute_timer = null
}
}

protected _schedule_recompute_all_models(): void {
const timeout = this._recompute_timeout
if (isNaN(timeout) || timeout <= 0) {
this._recompute_all_models()
} else if (isFinite(timeout)) {
this._cancel_recompute_all_models()
this._recompute_timer = setTimeout(() => {
this._recompute_all_models()
}, timeout)
}
}

protected _recompute_all_models(): void {
this._cancel_recompute_all_models()

let new_all_models_set = new Set<HasProps>()
for (const r of this._roots) {
new_all_models_set = sets.union(new_all_models_set, r.references())
Expand All @@ -269,7 +292,20 @@ export class Document implements Equatable {
model.attach_document(this)
this._new_models.add(model)
}
this._all_models = recomputed as any // XXX
this._all_models = recomputed
}

partially_update_all_models(value: unknown): void {
const refs = new Set<HasProps>()
HasProps._value_record_references(value, refs, {recursive: false})
for (const ref of refs) {
if (!this._all_models.has(ref.id)) {
ref.attach_document(this)
this._new_models.add(ref)
this._all_models.set(ref.id, ref)
}
}
this._schedule_recompute_all_models()
}

roots(): HasProps[] {
Expand Down Expand Up @@ -545,8 +581,17 @@ export class Document implements Equatable {
}

apply_json_patch(patch: Patch, buffers: Map<ID, ArrayBuffer> = new Map()): void {
this._hold_models_freeze = true // try ... finally
const {_hold_models_freeze} = this
this._hold_models_freeze = true
try {
this._apply_json_patch(patch, buffers)
} finally {
this._hold_models_freeze = _hold_models_freeze
}
this._schedule_recompute_all_models()
}

protected _apply_json_patch(patch: Patch, buffers: Map<ID, ArrayBuffer> = new Map()): void {
const finalize = (obj: HasProps) => {
obj.attach_document(this)
this._new_models.add(obj)
Expand Down Expand Up @@ -610,7 +655,5 @@ export class Document implements Equatable {
}
}
}

this._hold_models_freeze = false
}
}
5 changes: 4 additions & 1 deletion bokehjs/src/lib/model.ts
View file 6D4F
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,16 @@ export abstract class Model extends HasProps {
}

protected override _doc_attached(): void {
super._doc_attached()

if (this.js_event_callbacks.size != 0 || this.subscribed_events.size != 0) {
this._update_event_callbacks()
}
}

protected override _doc_detached(): void {
this.document!.event_manager.subscribed_models.delete(this)
super._doc_detached()
this.document?.event_manager.subscribed_models.delete(this)
}

select<T extends HasProps>(selector: ModelSelector<T>): T[] {
Expand Down
14 changes: 7 additions & 7 deletions bokehjs/test/unit/document/document.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ describe("Document", () => {
})

it("tracks all_models", () => {
const d = new Document()
const d = new Document({recompute_timeout: NaN})
expect(d.roots().length).to.be.equal(0)
expect(d.all_models.size).to.be.equal(0)
const m = new SomeModel()
Expand All @@ -220,7 +220,7 @@ describe("Document", () => {
})

it("tracks all_models with list property", () => {
const d = new Document()
const d = new Document({recompute_timeout: NaN})
expect(d.roots().length).to.be.equal(0)
expect(d.all_models.size).to.be.equal(0)
const m = new SomeModelWithChildren()
Expand All @@ -247,7 +247,7 @@ describe("Document", () => {
})

it("tracks all_models with list property where list elements have a child", () => {
const d = new Document()
const d = new Document({recompute_timeout: NaN})
expect(d.roots().length).to.be.equal(0)
expect(d.all_models.size).to.be.equal(0)
const m = new SomeModelWithChildren()
Expand Down Expand Up @@ -320,7 +320,7 @@ describe("Document", () => {
})

it("can have all_models with multiple references", () => {
const d = new Document()
const d = new Document({recompute_timeout: NaN})
expect(d.roots().length).to.be.equal(0)
expect(d.all_models.size).to.be.equal(0)

Expand Down Expand Up @@ -354,7 +354,7 @@ describe("Document", () => {
})

it("can have all_models with cycles", () => {
const d = new Document()
const d = new Document({recompute_timeout: NaN})
expect(d.roots().length).to.be.equal(0)
expect(d.all_models.size).to.be.equal(0)

Expand All @@ -380,7 +380,7 @@ describe("Document", () => {
})

it("can have all_models with cycles through lists", () => {
const d = new Document()
const d = new Document({recompute_timeout: NaN})
expect(d.roots().length).to.be.equal(0)
expect(d.all_models.size).to.be.equal(0)

Expand Down Expand Up @@ -750,7 +750,7 @@ describe("Document", () => {
})

it("can patch a reference property", () => {
const d = new Document()
const d = new Document({recompute_timeout: NaN})
expect(d.roots().length).to.be.equal(0)
expect(d.all_models.size).to.be.equal(0)

Expand Down
Loading
0