Fix faulty write buffer behavior when sending large data chunks over TLS (Mac OS X only)

[clue]

This changeset fixes faulty write buffer error handling when sending larger data chunks over TLS (HTTPS) streams as exhibited on Mac OS X only. This component will now only report a write error when nothing could be sent and and error has been reported. This indicates a fatal error condition, whereas all other cases mean that a write should be retried according to how the loop reports the stream as writable.

Supersedes / closes #149, thanks @WyriHaximus and @mpociot for reporting and filing an initial work-around.
Refs #105, #132 and reactphp/http-client#109

[clue]

This is now ready for review

The build output is a bit harder to read because Mac OS X build errors are being ignored (#112). The first commit and build introduces a reproduction case and fails as expected. The second commit fixes this issue as documented above and successfully builds also on Mac OS X. The last commit simplifies the Mac OS X test setup slightly (see also #147 and #112).

Again thanks to @WyriHaximus and @mpociot for reporting and filing an initial work-around in #149. I've had a chat earlier today with @mpociot to verify my assumption and we've been able to reproduce and fix this some minimal changes to the existing test suite and code base. I have intentionally pushed this as multiple commits to highlight this exact issue.

[WyriHaximus]

Can we remove it from allowed failures as well? Since this now includes a fix for Mac OS of significant impact I would feel more secure that we secure we force it to also pass on OS X

[clue]

Yes we can.

The test suite works just fine on all platforms (including Mac OS X) before applying this PR and thereafter. I don't think this PR has any effect on whether Mac OS X build failures should be reported, but I can see that this could be useful in the long run.

We've put this on allow_failures because Travis' Mac OS X integration felt somewhat beta (#112), but I don't think we've seen significant problems since then, so I'm fine with it either way.

What do you think?

[WyriHaximus]

Agreed 👍

[trowski]

When this issue first presented itself, I recall attempting to use the error handler as well, so I was very curious to see why that approach appeared to work here but not in Amp.

I committed a similar approach in a branch of Amp's byte-stream package: amphp/byte-stream@655ea5c. However, I'm getting test failures now due to EAGAIN in 7.3 and below under some conditions: macOS Travis build. IIRC, that is why I didn't use the error handler approach.

[clue]

When this issue first presented itself, I recall attempting to use the error handler as well, so I was very curious to see why that approach appeared to work here but not in Amp.

I committed a similar approach in a branch of Amp's byte-stream package: amphp/byte-stream@655ea5c. However, I'm getting test failures now due to EAGAIN in 7.3 and below under some conditions: macOS Travis build. IIRC, that is why I didn't use the error handler approach.

Interesting to see how you've tried to apply the same concept to Amp and how the behavior differs from what we're seeing here 👍 I took a quick glimpse and it looks like the major difference is that you're performing multiple fwrite() calls in a single writable event whereas we concatenate all outgoing data and only fwrite() once when the loop indicates a writable event. Using this approach, we can not produce any problems any more, both in our test suite and some real-world tests, so I think this is a reasonable improvement over the old situation for us.

Happy to help if you have any questions, but I'd like to ask you to please give credit where credit is due (we're looking at >6 hours of work in this PR).

[trowski]

I took a quick glimpse and it looks like the major difference is that you're performing multiple fwrite() calls in a single writable event whereas we concatenate all outgoing data and only fwrite() once when the loop indicates a writable event.

I appreciate you taking a look into what was different here. I didn't realize React was concatenating writes. Since our abstraction has separate promises for each write, we were also performing each fwrite separately. I took the simple approach for a fix by only failing if the first write fails: amphp/byte-stream@0dd0eb8

Happy to help if you have any questions, but I'd like to ask you to please give credit where credit is due (we're looking at >6 hours of work in this PR).

Not a problem. I also spent hours debugging when I first discovered the issue through our HTTP server. I put a link to this issue next to the related code and on the added test.