Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlocks on failures with parTraverse and parSequence #2258

Merged
merged 8 commits into from Nov 13, 2018

Conversation

@ngbinh
Copy link
Contributor

@ngbinh ngbinh commented Nov 8, 2018

Both of these tests fail by timing out on my local machine:

[info] BlazeClientSpec                                                                                     
[info] Blaze Http1Client should                                     
[info]   + raise error NoConnectionAllowedException if no connections are permitted for key (789 ms)
[info]   + make simple https requests (966 ms)                                                                                                      
[info]   + behave and not deadlock (1 second, 34 ms)                                         
[error]   x behave and not deadlock on failures with parTraverse (30 seconds, 245 ms)                      
[error]    None is not Some (BlazeClientSpec.scala:151)                               
[error] org.http4s.client.blaze.BlazeClientSpec.$anonfun$new$18(BlazeClientSpec.scala:151)
[error]   x behave and not deadlock on failures with parSequence (30 seconds, 308 ms)                              
[error]    None is not Some (BlazeClientSpec.scala:184)                                                                                  
[error] org.http4s.client.blaze.BlazeClientSpec.$anonfun$new$31(BlazeClientSpec.scala:184)                                                                                                                  
[info]   + obey response header timeout (30 seconds, 374 ms)                                                                                                                                                
[info]   + unblock waiting connections (30 seconds, 417 ms)                                                                                                       
[info]   + reset request timeout (30 seconds, 462 ms)                  
[info]   + drain waiting connections after shutdown (1 second, 140 ms)                                                                   
[info]   + cancel infinite request on completion (1 second, 193 ms)                                                                                                                                         
[info] Total for specification BlazeClientSpec                  
[info] Finished in 31 seconds, 904 ms                    
[info] 10 examples, 38 expectations, 2 failures, 0 error  
ngbinh added 2 commits Nov 8, 2018
@ngbinh
Copy link
Contributor Author

@ngbinh ngbinh commented Nov 8, 2018

@rossabaker should this be reopened?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Nov 9, 2018

Yes, I mentioned it in the other ticket and it autoclosed.

@rossabaker rossabaker reopened this Nov 9, 2018
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Nov 11, 2018

After 212d7fa, the parTraverse test still fails for me due to #2265. This fixes the deadlocks.

@rossabaker rossabaker changed the title [DO NOT MERGE] Unit tests for #2251 Fix deadlocks on failures with parTraverse and parSequence Nov 11, 2018
_ <- failedRequests.handleErrorWith(_ => IO.unit)
_ <- failedRequests.handleErrorWith(_ => IO.unit)
_ <- failedRequests.handleErrorWith(_ => IO.unit)
_ <- failedRequests.handleErrorWith(_ => IO.unit)
_ <- failedRequests.handleErrorWith(_ => IO.unit)

This comment has been minimized.

@ngbinh

ngbinh Nov 11, 2018
Author Contributor

I remember there are cases where we need to flatmap two failures to reproduce the error.

This comment has been minimized.

@rossabaker

rossabaker Nov 11, 2018
Member

Travis is a bit underpowered, so hitting it too hard can lead to spurious failures, so we'll have to find the right balance here. I'll bump it back up, and we'll see if we can turn it back down.

It passed, when it fails most of the time for me locally on #2265. Some bugs are exposed only by slow Travis and some bugs are hidden by slow Travis. 😩

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Nov 12, 2018

The last commit should also fix #2265.

@ngbinh
Copy link
Contributor Author

@ngbinh ngbinh commented Nov 13, 2018

looks great to me. Thanks!

@rossabaker rossabaker merged commit a264f31 into http4s:master Nov 13, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ngbinh ngbinh deleted the ngbinh:unit-test-2251 branch Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants