tumblr / colossus

I/O and Microservice library for Scala
Apache License 2.0
1.14k stars 96 forks source link

Fixing a bug with handling unexpected exceptions in Controller #517

Closed DanSimon closed 7 years ago

DanSimon commented 7 years ago

This fixes a bug uncovered by another bug that will be in a separate PR against master. Basically when ServiceClient threw an exception, it got caught by InputController as an unexpected error resulting in killing the connection (which it should do). The problem was that it was calling disconnect instead of kill, which meant ServiceClient would not handle the disconnect as an error and would not try to reconnect.

The solution was a little more involved because in the case of server connections, calling disconnect() is actually what we want to do, because when pipelining responses we want the server connection to be able to send back any responses for successfully processed requests before closing the connection.

So the solution is to basically punt the decision of which function to call to the downstream handler, aka ServiceServer or ServiceClient. So now the onFatalError event handler method is required to return a FatalErrorAction which instructs the controller how to behave.

codecov-io commented 7 years ago

Codecov Report

Merging #517 into develop will decrease coverage by 0.04%. The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #517      +/-   ##
==========================================
- Coverage    84.74%   84.7%   -0.05%     
==========================================
  Files           98      98              
  Lines         4288    4289       +1     
  Branches       348     342       -6     
==========================================
- Hits          3634    3633       -1     
- Misses         654     656       +2
Impacted Files Coverage Δ
...in/scala/colossus/streaming/StreamTranscoder.scala 84.21% <ø> (ø) :arrow_up:
...ossus/protocols/http/streaming/StreamService.scala 82.35% <0%> (-10.99%) :arrow_down:
...scala/colossus/protocols/websocket/Websocket.scala 91.07% <0%> (-0.83%) :arrow_down:
...rc/main/scala/colossus/service/ServiceClient.scala 94.81% <100%> (-0.04%) :arrow_down:
...rc/main/scala/colossus/controller/Controller.scala 83.33% <100%> (+8.33%) :arrow_up:
...rc/main/scala/colossus/service/ServiceServer.scala 92.78% <100%> (-0.08%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2d3610e...8755152. Read the comment docs.

benblack86 commented 7 years ago

👍

Should we wait for the mapTry fix before merging and cutting RC4?

DanSimon commented 7 years ago

This can be merged now, but we should wait for the mapTry fix to do the release. That one should be much more straight-forward so I should have a PR ready soon.