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.

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

Review: Async.Promise #567

Merged
merged 20 commits into from
Jan 1, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
dd78349
Async.Promise: First implementation based on es6-promise
rhysd Jun 10, 2017
088e312
Async.Promise: Add tests for all APIs
rhysd Dec 17, 2017
ad71bbe
Async.Promise: Do not check self fulfillment
rhysd Dec 19, 2017
78a7476
Async.Promise: Ensure fulfilled/rejected callbacks are asynchronously…
rhysd Dec 23, 2017
ba7d405
Async.Promise: Fix tests for asynchronous fulfillment/rejection
rhysd Dec 23, 2017
1e82791
Async.Promise: Prefer partial to lambda
rhysd Dec 23, 2017
b0e9d8f
Async.Promise: Add a help document
rhysd Dec 24, 2017
fea1b27
Async.Promise: Fix maybe-uninitialized variable
rhysd Dec 26, 2017
264c07f
Async.Promise: Prefer function to method
rhysd Dec 26, 2017
f392520
Async.Promise: Fix job example in document
rhysd Dec 27, 2017
58041c4
Async.Promise: Explicit l: at :let to avoid functioin name conflicts
rhysd Dec 27, 2017
e956ca0
Async.Promise: Give tags to examples and fix example codes
rhysd Dec 27, 2017
057d36e
Async.Promise: Refactor state check in _publish() and wait_group in a…
rhysd Dec 27, 2017
cf1bd23
Async.Promise: Check features rather than versions for availability
rhysd Dec 27, 2017
427a0cd
Async.Promise: Fix test descriptions and variable names, more checks
rhysd Dec 28, 2017
82b5c31
Async.Promise: Promise value is never settled in _subscribe()
rhysd Dec 28, 2017
e9770e2
Async.Promise: Reject with dict when catching an exception
rhysd Dec 29, 2017
1095e9b
Async.Promise: Fix/brush up the help document
rhysd Dec 30, 2017
b8770b7
Async.Promise: Use themis.vim v1.5.4
rhysd Dec 31, 2017
fbed16a
Async.Promise: Prefer `is v:null` to `== type(v:null)`
rhysd Dec 31, 2017
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ If you are a Vim plugin author, please check this out.
Module | Description
------------------------------------------------ | ------------------------------
[Assertion](doc/vital/Assertion.txt) | assertion library
[Async.Promise](doc/vital/Async/Promise.txt) | An asynchronous operation like ES6 Promise
[Bitwise](doc/vital/Bitwise.txt) | bitwise operators
[ConcurrentProcess](doc/vital/ConcurrentProcess.txt) | manages processes concurrently with vimproc
[Data.Base64](doc/vital/Data/Base64.txt) | base64 utilities library
Expand Down
257 changes: 257 additions & 0 deletions autoload/vital/__vital__/Async/Promise.vim
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
" ECMAScript like Promise library for asynchronous operations.
" Spec: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
" This implementation is based upon es6-promise npm package.
" Repo: https://github.com/stefanpenner/es6-promise

" States of promise
let s:PENDING = 0
let s:FULFILLED = 1
let s:REJECTED = 2
Copy link
Member

Choose a reason for hiding this comment

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

提案
ECMA script では Promise.race()で Promise の状態取得が出来ますが Vim script は関数呼び出しのオーバーヘッドが大きいので上記の定数を s:_vital_created(module) を使ってモジュール定数として定義し s:PROMISE._state を公開アトリビュートとする(必要であれば lockvar も?)のはどうでしょうか?

" 使用感イメージ
let p1 = Promise.resolve()
let p2 = Promise.reject('foobar')
let p3 = Promise.new({ resolve -> execute('echo "Do nothing"') })

echo p1.state == Promise.FULFILLED
echo p2.state == Promise.REJECTED
echo p3.state == Promise.PENDING

Copy link
Contributor Author

Choose a reason for hiding this comment

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

すみません,#526 に1文だけ書いてこの PR には書いていなかったのですが,この PR は ES6 の Promise の API の実装にとどめる形にさせてください.理由は,

  • bluebird のような強力な API を実装することも考えられるため,今入れたい API を言い始めるとキリが無さそう
  • まずは最小の状態で master にマージして実戦投入して,real world で本当に必要な API だけを足したい

ためです.PR の本文にも追記しておきます💪


なのでここからは一応オフトピですが,せっかく提案いただいたので僕の考えを書いておきます.

一般的に,非同期処理で実行の状態を見て処理を分岐するのはバッドプラクティスだと思います.例えば JavaScript には VM 上で他の非同期に走っているコンテキストの状態を取る手段はありませんし,Go ではあえてゴルーチン(coroutine)の状態や ID,今自分がどのゴルーチン(コンテキスト)で実行されているかを取る API を提供していません.
また,飽くまで僕自身の経験としてですが,Promise の状態を取りたいと思ったことが無いというのもあります.

ただ,bluebird のように Promise の 3rd party 実装では Promise の状態を同期的に introspection できるものもあるので,real world で本当に必要なユースケースが発生した時に入れることを考えて議論するのが良いかなと思います.

Copy link
Member

Choose a reason for hiding this comment

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

ご説明ありがとうございます。納得しかない


let s:DICT_T = type({})

" @vimlint(EVL103, 1, a:resolve)
" @vimlint(EVL103, 1, a:reject)
function! s:noop(resolve, reject) abort
endfunction
" @vimlint(EVL103, 0, a:resolve)
" @vimlint(EVL103, 0, a:reject)
let s:NOOP = function('s:noop')

" Internal APIs

let s:PROMISE = {
\ '_state': s:PENDING,
\ '_children': [],
\ '_fulfillments': [],
\ '_rejections': [],
\ '_result': v:null,
\ }

let s:id = -1
function! s:_next_id() abort
let s:id += 1
return s:id
endfunction

" ... is added to use this function as a callback of timer_start()
function! s:_invoke_callback(settled, promise, callback, result, ...) abort
let has_callback = a:callback isnot v:null
let success = 1
let err = v:null
if has_callback
try
let l:Result = a:callback(a:result)
catch
let err = {
\ 'exception' : v:exception,
\ 'throwpoint' : v:throwpoint,
\ }
let success = 0
endtry
else
let l:Result = a:result
endif

if a:promise._state != s:PENDING
" Do nothing
elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, err)
elseif a:settled == s:FULFILLED
call s:_fulfill(a:promise, Result)
elseif a:settled == s:REJECTED
call s:_reject(a:promise, Result)
endif
Copy link
Member

Choose a reason for hiding this comment

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

💭 [IMO]
ちょっと読んでて混乱しました。
最初の pending の判定を除くと、上2つが has_callbackTRUE の場合、下 2 つが has_callbackFALSE の場合なので、L43 の if の中にそれぞれ書いてしまっても良いかも。
その場合 pending の判定が重複しちゃうのでどちらが良いかむずいですが… 中に入れてしまえば L54 の不要な代入も自然に減らせそうな気がします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かにここはちょっと冗長かつ微妙な実装になっているのですが,意図としてはコールバックの発火処理と次の(子供の)Promise へチェーンする処理を分離する実装になっています.

a:promise._state != s:PENDING をダブらせて上の if の中で判定するというのも考えたのですが,コールバック実行部分とチェーンの続きを発火する部分が混ざると,それはそれで結構読みづらい印象でした.

endfunction

" ... is added to use this function as a callback of timer_start()
function! s:_publish(promise, ...) abort
let settled = a:promise._state
if settled == s:PENDING
throw 'vital: Async.Promise: Cannot publish a pending promise'
endif

if empty(a:promise._children)
return
endif

for i in range(len(a:promise._children))
if settled == s:FULFILLED
let l:CB = a:promise._fulfillments[i]
else
" When rejected
let l:CB = a:promise._rejections[i]
endif
let child = a:promise._children[i]
if child isnot v:null
call s:_invoke_callback(settled, child, l:CB, a:promise._result)
else
call l:CB(a:promise._result)
endif
endfor

let a:promise._children = []
let a:promise._fulfillments = []
let a:promise._rejections = []
endfunction

function! s:_subscribe(parent, child, on_fulfilled, on_rejected) abort
let a:parent._children += [ a:child ]
let a:parent._fulfillments += [ a:on_fulfilled ]
let a:parent._rejections += [ a:on_rejected ]
endfunction

function! s:_handle_thenable(promise, thenable) abort
if a:thenable._state == s:FULFILLED
call s:_fulfill(a:promise, a:thenable._result)
elseif a:thenable._state == s:REJECTED
call s:_reject(a:promise, a:thenable._result)
else
call s:_subscribe(
\ a:thenable,
\ v:null,
\ function('s:_resolve', [a:promise]),
\ function('s:_reject', [a:promise]),
\ )
endif
endfunction

function! s:_resolve(promise, ...) abort
let l:Result = a:0 > 0 ? a:1 : v:null
if s:is_promise(Result)
call s:_handle_thenable(a:promise, Result)
else
call s:_fulfill(a:promise, Result)
endif
endfunction

function! s:_fulfill(promise, value) abort
if a:promise._state != s:PENDING
return
endif
let a:promise._result = a:value
let a:promise._state = s:FULFILLED
if !empty(a:promise._children)
call timer_start(0, function('s:_publish', [a:promise]))
endif
endfunction

function! s:_reject(promise, ...) abort
if a:promise._state != s:PENDING
return
endif
let a:promise._result = a:0 > 0 ? a:1 : v:null
let a:promise._state = s:REJECTED
call timer_start(0, function('s:_publish', [a:promise]))
endfunction

function! s:_notify_done(wg, index, value) abort
let a:wg.results[a:index] = a:value
let a:wg.remaining -= 1
if a:wg.remaining == 0
call a:wg.resolve(a:wg.results)
endif
endfunction

function! s:_all(promises, resolve, reject) abort
let total = len(a:promises)
if total == 0
call a:resolve([])
return
endif

let wait_group = {
\ 'results': repeat([v:null], total),
\ 'resolve': a:resolve,
\ 'remaining': total,
\ }

" 'for' statement is not available here because iteration variable is captured into lambda
" expression by **reference**.
call map(
\ copy(a:promises),
\ {i, p -> p.then({v -> s:_notify_done(wait_group, i, v)}, a:reject)},
\ )
endfunction

function! s:_race(promises, resolve, reject) abort
for p in a:promises
Copy link
Member

Choose a reason for hiding this comment

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

p は Funcref になり得るので l:P がいいと思います

Copy link
Member

Choose a reason for hiding this comment

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

あれ ps:PROMISE オブジェクトのコピー(インスタンス)な気がします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lambdalisue さんのおっしゃる通り,p は dict なので小文字で大丈夫です.

Copy link
Member

Choose a reason for hiding this comment

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

あ、確かに。了解です。

call p.then(a:resolve, a:reject)
endfor
Copy link
Member

Choose a reason for hiding this comment

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

確認
ここも同様に map(..., 'expr') タイプの方が 1.5 倍ほど早いですがよろしいでしょうか?

https://gist.github.com/lambdalisue/7cbaa0ece9496ec8052f5a86d33d8f17#file-test-vim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ベンチマークを測った上で検討してみます.正直,.then のオーバーヘッドのほうが大きくてほぼ影響ないんじゃないかという気もしています.

Copy link
Member

Choose a reason for hiding this comment

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

上記のベンチマークは 100000 回のループなので無視しても良いと思っています。
そのため「確認」としました

endfunction

" Public APIs

function! s:new(resolver) abort
let promise = deepcopy(s:PROMISE)
let promise._vital_promise = s:_next_id()
try
if a:resolver != s:NOOP
call a:resolver(
\ function('s:_resolve', [promise]),
\ function('s:_reject', [promise]),
\ )
endif
catch
call s:_reject(promise, {
\ 'exception' : v:exception,
\ 'throwpoint' : v:throwpoint,
\ })
endtry
return promise
endfunction

function! s:all(promises) abort
return s:new(function('s:_all', [a:promises]))
endfunction

function! s:race(promises) abort
return s:new(function('s:_race', [a:promises]))
endfunction

function! s:resolve(...) abort
let promise = s:new(s:NOOP)
call s:_resolve(promise, a:0 > 0 ? a:1 : v:null)
Copy link
Member

Choose a reason for hiding this comment

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

[nit] get(a:000, 0, v:null) とか他のところで使ってるし書き方統一してもよさそう。

(個人的には get(a:, 1, v:null) が好みだけど統一されてたらなんでもok)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

個人的な好みにより a:0 になりました

Copy link
Member

Choose a reason for hiding this comment

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

[感想] 自分がいちばん好みじゃないの来た...! (勿論直してほしいとかではなくて単なる感想で、言い分としてはa:0は考えることが増えるのと(a:0が数でぇ...+1を見に行く...)、ほかの get or default イディオムと合わせたいので自分は get(a:, n, default) を使ってる (g: とかと一緒のイディオムにする)

return promise
endfunction

function! s:reject(...) abort
let promise = s:new(s:NOOP)
call s:_reject(promise, a:0 > 0 ? a:1 : v:null)
return promise
endfunction

function! s:is_available() abort
return has('lambda') && has('timers')
endfunction

function! s:is_promise(maybe_promise) abort
return type(a:maybe_promise) == s:DICT_T && has_key(a:maybe_promise, '_vital_promise')
endfunction

function! s:_promise_then(...) dict abort
let parent = self
let state = parent._state
let child = s:new(s:NOOP)
let l:Res = a:0 > 0 ? a:1 : v:null
let l:Rej = a:0 > 1 ? a:2 : v:null
if state == s:FULFILLED
call timer_start(0, function('s:_invoke_callback', [state, child, Res, parent._result]))
elseif state == s:REJECTED
call timer_start(0, function('s:_invoke_callback', [state, child, Rej, parent._result]))
else
call s:_subscribe(parent, child, Res, Rej)
endif
return child
endfunction
let s:PROMISE.then = function('s:_promise_then')
Copy link
Member

Choose a reason for hiding this comment

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

質問
辞書に対して直接関数定義を行わないで、一度スクリプト関数として定義してから functionfuncref 化して辞書に代入しているのには何か理由があるのでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここは以前ツイッターで @haya14busa さんに指摘されたのですが,辞書関数だとエラーが出た時のコールバックの関数名が数字になってしまってデバッグが困難になるためです.

Copy link
Member

Choose a reason for hiding this comment

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

なるほど納得。でもデバッグの事情の為にこういうハックするのは少しモニョりますね。今回のケースだと非同期なのでデバッグの簡潔さを重要視した方が良いですが‥‥

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コードが若干複雑になってしまうのは確かにあると思います.

Copy link
Member

Choose a reason for hiding this comment

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

実は vital core でもやってます

let s:Vital.import = s:_function('s:import')

書くの面倒だという以外はいいことしかないと思ってる


" .catch() is just a syntax sugar of .then()
function! s:_promise_catch(...) dict abort
return self.then(v:null, a:0 > 0 ? a:1 : v:null)
endfunction
let s:PROMISE.catch = function('s:_promise_catch')

" vim:set et ts=2 sts=2 sw=2 tw=0:
1 change: 1 addition & 0 deletions doc/vital.txt
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ LINKS *Vital-links*

Vital modules *Vital-modules*
|vital/Assertion.txt| assertion library.
|vital/Async/Promise.txt| An asynchronous operation like ES6 Promise
|vital/Bitwise.txt| bitwise operators.
|vital/ConcurrentProcess.txt| Manages processes concurrently with vimproc.
|vital/Data/Base64.txt| base64 utilities library.
Expand Down
Loading