Conversation
kanongil
left a comment
kanongil
left a comment
There was a problem hiding this comment.
Looks good, especially if updated to better accommodate "slow" onPostResponse handlers, that can be used to apply tail logic.
| const realm = ext.realm; | ||
| const bind = ext.bind || realm.settings.bind; | ||
| const response = await this._core.toolkit.execute(ext.func, this, { bind, realm, timeout: ext.timeout, name: event.type }); | ||
| const response = await this._core.toolkit.execute(ext.func, this, { bind, realm, timeout: ext.timeout, name: event.type, ignoreResponse: options.ignoreResponse }); | ||
|
|
||
| if (options.ignoreResponse) { | ||
| if (Boom.isBoom(response)) { | ||
| this._log(['ext', 'error'], response); | ||
| } | ||
|
|
||
| continue; | ||
| } |
There was a problem hiding this comment.
This logic needs to be executed in parallel for onPostResponse handlers. Otherwise the timing can get messed up.
This will make onPostResponse a good place to apply tail logic, allowing errors to be logged and all.
There was a problem hiding this comment.
I don't follow. Can you give an example where serial processing is going to be a problem here (and how is it different from current response event)?
There was a problem hiding this comment.
Say you add an onPostResponse handler to increment a value in your db, and a handler to log to stdout. The log handler would be delayed an indeterminate amount of time while the previous handler talks to the db. This will in turn mean that the log entry will appear later than intended, and that multiple such log entries could appear out of order.
Also, the current response event is essentially a parallel handler, since it is a fire and forget thing.
There was a problem hiding this comment.
The await lets you enforce order but it is not required. There is nothing blocking you for implementing extension handlers that return right away and schedule work for a next tick. I'm inclined not to special case this to parallel execution because it makes reasoning about ext order harder. Keeping it the same means the same plugin order logic applies, etc.
It is probably worth documenting it so that people will write smarter handlers if they rely on getting quick notification of the event and then doing something.
There was a problem hiding this comment.
It makes sense that you want to keep the existing reasoning about how it runs, though I believe it can cause issues.
Maybe an extension isn't the right fit for this? Most of the h properties / methods doesn't make sense anyway.
There was a problem hiding this comment.
It's not a perfect fit but it benefits for a lot of other similar facilities that I think it still works. We'll see how it goes.
2 participants
This will eventually replace the
responseevent (removed next major). Related to feedback from #4053.