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.

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

disposer not called when resource is null #1099

Closed
iSOcH opened this Issue May 13, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@iSOcH

iSOcH commented May 13, 2016

(NodeJS 6.1, Bluebird 3.3.5)

I tried using Promise.using with locks from rwlock. Imho in that scenario I do not really have a resource that i need to pass to my code, so I went with (removed everything related to rwlock to show the bluebird relevant part only):

var Promise = require("bluebird");

function writeLockDisposer() {
  return Promise
    .resolve(null) // will work instead: .resolve({})
    .disposer(function () { console.log("won't see me :/"); });
};

function withWriteLock(fn) {
  return Promise.using(writeLockDisposer(), fn);
};

withWriteLock(function () {
  return Promise.resolve("you'll see me");
}).then(function () {
  console.log("and you'll also see mee");
});

I'm rather new to JS and Node, so I might just not know some conventions like "you never fulfill a promise with null", but I found this behaviour somewhat strange after reading the documentation:

In conjunction with .disposer, using will make sure that no matter what, the specified disposer will be called when the promise returned by the callback passed to using has settled.

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion May 13, 2016

Collaborator

Looks like we're using null inappropriately there:

bluebird/src/using.js

Lines 65 to 82 in c0d4472

Disposer.prototype.resource = function () {
if (this.promise().isFulfilled()) {
return this.promise().value();
}
return null;
};
Disposer.prototype.tryDispose = function(inspection) {
var resource = this.resource();
var context = this._context;
if (context !== undefined) context._pushContext();
var ret = resource !== null
? this.doDispose(resource, inspection) : null;
if (context !== undefined) context._popContext();
this._promise._unsetDisposable();
this._data = null;
return ret;
};

Collaborator

spion commented May 13, 2016

Looks like we're using null inappropriately there:

bluebird/src/using.js

Lines 65 to 82 in c0d4472

Disposer.prototype.resource = function () {
if (this.promise().isFulfilled()) {
return this.promise().value();
}
return null;
};
Disposer.prototype.tryDispose = function(inspection) {
var resource = this.resource();
var context = this._context;
if (context !== undefined) context._pushContext();
var ret = resource !== null
? this.doDispose(resource, inspection) : null;
if (context !== undefined) context._popContext();
this._promise._unsetDisposable();
this._data = null;
return ret;
};

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov May 16, 2016

Owner

Well, if we used undefined then it would not be called when resource is undefined... the example doesn't show that writeLockDisposer is allocating any resource, and if it does, how do you unallocate it if you don't get any handle?

Owner

petkaantonov commented May 16, 2016

Well, if we used undefined then it would not be called when resource is undefined... the example doesn't show that writeLockDisposer is allocating any resource, and if it does, how do you unallocate it if you don't get any handle?

@iSOcH

This comment has been minimized.

Show comment
Hide comment
@iSOcH

iSOcH May 17, 2016

Thanks for your feedback.

I can release the lock using a function I get passed from rwlock. That function should not be passed to the consuming code though (the argument to withWriteLock).

Here's my actual code (its TS):

import * as Promise from "bluebird";
import * as Rwlock from "rwlock";

class Library {
    private _lock = new Rwlock();
    private _ReportMeta: typeof Reports.ReportMeta;

    constructor(private path: string) {
        // TODO check if folder exists and is read+writable
        this._ReportMeta = Reports.init(this.path);
    }

    listReports(): Promise<Reports.ReportMeta[]> {
        return this.withWriteLock(() => {
            const r = Math.random();
            if (r < 1 / 3) {
                return Promise.resolve([]);
            } else if (r < 2 / 3) {
                throw "exception";
            } else {
                return Promise.reject("crashed");
            }
        });
    }

    private writeLockDisposer(): Promise.Disposer<void> {
        var releaseLock: Rwlock.Release; // this is just a parameterless void function

        return new Promise((res, rej) => {
            this._lock.writeLock(relFn => {
                releaseLock = relFn;
                res({}); // the releaseLock-Function is not a ressource, it does not make sense to pass it to "user-code"
            });
        }).disposer(releaseLock);
    }

    private withWriteLock<R, T>(fn: (r: R) => Promise<T>): Promise<T> {
        return Promise.using<R, T>(this.writeLockDisposer(), fn);
    }
}

iSOcH commented May 17, 2016

Thanks for your feedback.

I can release the lock using a function I get passed from rwlock. That function should not be passed to the consuming code though (the argument to withWriteLock).

Here's my actual code (its TS):

import * as Promise from "bluebird";
import * as Rwlock from "rwlock";

class Library {
    private _lock = new Rwlock();
    private _ReportMeta: typeof Reports.ReportMeta;

    constructor(private path: string) {
        // TODO check if folder exists and is read+writable
        this._ReportMeta = Reports.init(this.path);
    }

    listReports(): Promise<Reports.ReportMeta[]> {
        return this.withWriteLock(() => {
            const r = Math.random();
            if (r < 1 / 3) {
                return Promise.resolve([]);
            } else if (r < 2 / 3) {
                throw "exception";
            } else {
                return Promise.reject("crashed");
            }
        });
    }

    private writeLockDisposer(): Promise.Disposer<void> {
        var releaseLock: Rwlock.Release; // this is just a parameterless void function

        return new Promise((res, rej) => {
            this._lock.writeLock(relFn => {
                releaseLock = relFn;
                res({}); // the releaseLock-Function is not a ressource, it does not make sense to pass it to "user-code"
            });
        }).disposer(releaseLock);
    }

    private withWriteLock<R, T>(fn: (r: R) => Promise<T>): Promise<T> {
        return Promise.using<R, T>(this.writeLockDisposer(), fn);
    }
}
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov May 17, 2016

Owner

Ok I guess we could use special object identity instead of null internally to free null to be usable

Owner

petkaantonov commented May 17, 2016

Ok I guess we could use special object identity instead of null internally to free null to be usable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment