search

toland / patron

Ruby HTTP client based on libcurl
http://toland.github.com/patron/
MIT License
541 stars 74 forks source link

Fix broken specs #172

Closed marshall-lee closed 5 years ago

marshall-lee commented 5 years ago

Puma migration in f905d35 didn't go well. Some specs are failing due to the following problems:

The reason that we don't see them failing is that part of the specs were even not running: some of the spec examples sets trap('INT') that does the global exit (added in ba2161c). One should never set trap handlers inside tests since they're global to the running process.

Such a trap handler overwrites the one set by RSpec (btw RSpec's SIGINT handler is already overwritten by Puma that's not good too).

If you run rspec ./spec/session_spec.rb you'd see:

Finished in 14.51 seconds (files took 0.46612 seconds to load)
23 examples, 0 failures

But that file contains more than 23 examples, it should be 88.

marshall-lee commented 5 years ago

@julik @toland

It seems that HTTPS doesn't work with fork() not only with SecureTransport but with OpenSSL too. I'm not sure it should really work with fork at all — sharing SSL state between processes always create problems.

also: https://stackoverflow.com/questions/15466809/libcurl-ssl-error-after-fork

UPD It works on OpenSSL, it was just a typo in spec: de3cd56.

Also I don't know how to fix specs that use /wrongcontentlength endpoint with HTTPS connection and I'm not sure they're really fixable.

julik commented 5 years ago

Ohlala

marshall-lee commented 5 years ago

I commented out two failing ssl specs which I doubt they're even repairable in 976c669. Also I make PatronTestServer to run in a separate process instead of thread because Puma and RSpec handles SIGINT differently: 8758b83.

marshall-lee commented 5 years ago

@toland @julik WDYT?

toland commented 5 years ago

It looks fine to me. However, at this point, @julik knows this code far better than I do. I will defer to him.

julik commented 5 years ago

I was out sick. This is some great work @marshall-lee , thank you. I didn't realise threading was so problematic in this configuration. I can't agree that removing Webrick tests was a bad idea though because it was exquisitely hard to test "slow" responses with it anyway.

marshall-lee commented 5 years ago

@julik I did all the final fixes

julik commented 5 years ago

Thank you for your changes!