Ensure http errors send a response

tswaters commented 8 years ago

Fixes #132

The test is a little bit messy due to complicated nature of making wreck (and by extension http.request) throw an http error. It passes though -- and without the change in lib/http.js it will timeout, so it does highlight the problem.

coveralls commented 8 years ago

Coverage increased (+0.3%) to 84.893% when pulling 56d70331b2c5d9b8457d09343e1c82bfe6eb04f9 on tswaters:issue-132 into 6b163bb5bb0d31b47be1ce8ddea5d360d24e3846 on senecajs:master.

coveralls commented 8 years ago

Coverage increased (+0.3%) to 84.893% when pulling 6a8f7338290b98e70aedc4970af296418cb6c74d on tswaters:issue-132 into 6b163bb5bb0d31b47be1ce8ddea5d360d24e3846 on senecajs:master.

coveralls commented 8 years ago

Coverage increased (+0.3%) to 84.893% when pulling 6a8f7338290b98e70aedc4970af296418cb6c74d on tswaters:issue-132 into 6b163bb5bb0d31b47be1ce8ddea5d360d24e3846 on senecajs:master.

Tapppi commented 7 years ago

Can this move forward? This bug is really painful because we cant test our wrapper.. This also affects calling dropped nodes in a mesh, because the actions will never timeout.

yossialush commented 7 years ago

Is there any chance to speed this up. This plugin is very foundemental and this bug has been causing us many ugly workarounds. In the meanwhile, can anyone suggest a proper workaround?

Tapppi commented 7 years ago

@rjrodger Please do not take this personally or as blame to you, I know maintaining OSS is hard and appreciate your work. I'm simply trying to outline a fundamental problem with this project and why we will be moving away from it.

I would love it if this was merged, but it hasn't even gotten a response in 6 months, so I think we will be forking all the seneca ecosystem modules we use and merge fixes to them while we are migrating away.

I appreciate the fact that you're writing the book and fixing examples for it and all that jazz, but seneca has proven unsustainable in production systems (at least for us). These issues highlight that there aren't enough maintainers and stability and unfortunately I don't see that changing once a project is this mature and with as big a code base as seneca has.

The incomplete documentation and weird behaviour (e.g. errors) was workable, but this PR has been the nail in the coffin. It took me at least 10x time to debug a service flooding another service with bursts that cause http errors, because of only getting this erratic error and nothing more.

We had a ton of cool internal fixes and changes to various seneca functionality, but we wont be even trying to get them into seneca core or open sourcing them, because it is obvious it is more trouble than it is worth for both of us. Over time our use of seneca has withered down to only communication, since everything else proved too cumbersome to use, either due to weird undocumented behavior or just no documentation. That problem is easier solved with other tools.

tswaters commented 7 years ago

@Tapppi there's nothing wrong with forking open source projects with fixes you have if things are not working for you. There's no need to suffer with the npm version if it has a bug you need fixed. Fork that bad-boy, apply the fix and get going. Not committing back useful changes in the form of PRs, though... that's not so good.

My organization has already forked most of the seneca ecosystem due to minor problems we've found, custom functionality we need that wouldn't fit with the generic use-cases of the plugins ,or just the needing to get rolling with software we're building that won't run due to problems.

seneca-user to add alternate login id - it references an internal field specific for our system so it doesn't make a lot of sense to send it back
seneca-web to add a lot of express-specific functionality around flashes, custom content types, etc. we've also given back a lot of PRs where the change makes sense.
- seneca-postgres-adapter doesn't supports schemas! that was a complete non-starter for us.
seneca-redis-store doesn't do expires. which is fine, so we found seneca-redis-store-expires but it hasn't been updated since seneca v1 so there was a bunch of things we needed to do to get it to go.
seneca-mesh / sneeze / swim to fix up a few problems, ensure everything is on the latest. fortunately all that stuff got merged.
seneca-transport -- see above. I'd consider it minor for now, it's when we start stress testing this will show its colours... much rather have a handled error and response sent instead of a warn in the logs.
seneca due to parambulator not providing all the bits it had to the error handlers (that one was merged -- https://github.com/senecajs/seneca/pull/577 -- should probably get back to the npm version of seneca)

Anyway, the important thing to note is that if we do find problems that genuinely warrant updates to the core project, we'll send it back and if it gets merged, great... if not, it doesn't really matter to us because we've already forked the projects and are committed to maintaining them internally.... but, it's our duty as OSS consumers to give back PRs where it makes sense.

senecajs / seneca-transport

Ensure http errors send a response #133