search

strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Json zombie streams #483

Closed Wizarth closed 4 years ago

Wizarth commented 4 years ago

Fixes https://github.com/strongloop/strong-remoting/issues/482 Includes test case.

slnode commented 4 years ago

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

bajtos commented 4 years ago

Thank you @Wizarth for the pull request, I appreciate the test case! I'll try to review the changes in the next few (working) days.

bajtos commented 4 years ago

@slnode ok to test

bajtos commented 4 years ago

@Wizarth Thank you again for the pull request. I added a new commit f7c066c where I have rewritten your test to use Node.js core API http.request, which gives us more control. Now the test returns a descriptive error message when the server event stream was not closed within 200ms (which is configurable).

I was not able to get rid of the mixed promise/callback style because of the way how streamClosed was created. We could rewrite this part from Promises to a regular EventEmitter, but I feel it's not worth the effort now.

It allowed me to remove your 50ms timer for calling req.abort(), now we abort the request just after we receive the response from the server. On my machine, the new test completes in ~40ms now.

On the other hand, the new test code is a bit longer than your original version. I think the benefits of faster execution and more robust implementation are worth it.

What do you think?

If you are fine with my changes, then we can proceed to squash the commits and land the pull request. Let me know!

bajtos commented 4 years ago

Also added 4986c52 to fix the new test on Node.js 8.x

Wizarth commented 4 years ago

Looks good to me.

I based the test on the existing test using streamClosed, which looks like it would have used the timeout as failure.

Using the http client response event is much better than a timer, I've never used that module directly.

bajtos commented 4 years ago

The fix is available in strong-remoting@3.16.2, thank you for the contribution! ❤️