Skip to content

Fix problem afterError always called - #281

Closed
florianorpeliere wants to merge 1 commit into
strongloop:2.x-latestfrom
florianorpeliere:2.x-latest
Closed

Fix problem afterError always called#281
florianorpeliere wants to merge 1 commit into
strongloop:2.x-latestfrom
florianorpeliere:2.x-latest

Conversation

@florianorpeliere

Copy link
Copy Markdown

Hello everyone.

I seem to have found a bug in version 2.25.
Indeed, the afterError hook is always called even if there is no error.
https://github.com/strongloop/strong-remoting/blob/2.x-latest/lib/remote-objects.js#L680
In fact, triggerErrorAndCallBack method is always called because phases.run use async.eachSeries and the documentation explain

each(arr, iterator, [callback])
callback(err) - Optional A callback which is called when all iterator functions have finished, or an error occurs.

In version 2.24 we had this
https://github.com/florianorpeliere/strong-remoting/blob/4d965966f4b860c27a6a42c421c7ea7e9d3696f5/lib/remote-objects.js#L625

So I allowed myself to put this piece of code in a method.
I do not know if the name really fits .

As this is my first contribution , please be indulgent .
Hoping to have helped you .

@slnode

slnode commented Feb 3, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos

bajtos commented Feb 5, 2016

Copy link
Copy Markdown
Member

@slnode ok to test

@bajtos

bajtos commented Feb 5, 2016

Copy link
Copy Markdown
Member

@florianorpeliere ouch, thank you for reporting the problem and sending a pull request to fix it.

Could you please add a unit-test to verify your change? The test should fail with the current implementation and pass with your proposed change in place.

@bajtos bajtos self-assigned this Feb 5, 2016
@bajtos

bajtos commented Feb 6, 2016

Copy link
Copy Markdown
Member

@florianorpeliere I landed a fix myself via #283 to get this regression fixed ASAP.

@florianorpeliere

Copy link
Copy Markdown
Author

@bajtos Okay no problem. Sorry I forgot the unit test.
But happy to have helped you.

@florianorpeliere
florianorpeliere deleted the 2.x-latest branch February 22, 2016 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants