Wrap all routes in toStrictEntity

[araspitzu]

This PR fixes #1030 by ensuring that the API parameters are parsed in a strict manner so without partially reading the data and stopping but ensuring that once we start reading we get the full package even if then it is rejected by the unmarshaller. The issue arises mostly when there are concurrent calls to the same endpoint and is a known behavior of akka, see issue.

[sstone]

I think it's the right solution, just taking a bit more time to review it.

[codecov-io]

Codecov Report

Merging #1032 into master will increase coverage by 0.2%.
The diff coverage is 65.41%.

@@            Coverage Diff            @@
##           master    #1032     +/-   ##
=========================================
+ Coverage   82.23%   82.43%   +0.2%     
=========================================
  Files          99       99             
  Lines        7559     7561      +2     
  Branches      291      296      +5     
=========================================
+ Hits         6216     6233     +17     
+ Misses       1343     1328     -15

Impacted Files	Coverage Δ
...e/src/main/scala/fr/acinq/eclair/api/Service.scala	69.93% <65.41%> (+0.37%)	⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	83.6% <0%> (+0.09%)	⬆️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala	75.23% <0%> (+0.3%)	⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala	86.76% <0%> (+0.94%)	⬆️
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala	96.15% <0%> (+3.84%)	⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala	78.49% <0%> (+4.3%)	⬆️
...lockchain/bitcoind/rpc/ExtendedBitcoinClient.scala	92.3% <0%> (+7.69%)	⬆️

[sstone]

Using toStrictEntity seems to be the "standard" solution to this problem (see comments on https://groups.google.com/forum/#!topic/akka-user/v-jNYPkKVCU, akka/akka-http#117) but there seems to be a performance impact (see eclipse-ditto/ditto#412) and some of the suggested fixes which are nearly 2 years old have not been merged.

I think using toStrictEntity, but with a shorter time-out (5 seconds instead of 30, since all our API requests are small) should be fine, but another option is just to avoid matching on multiple formField extractors and do something like:

                      path("connect") {
                        formFields("uri".as[NodeURI].?, nodeIdFormParam.?, "host".as[String].?, "port".as[Int].?) { (uri_opt, nodeId_opt, host_opt, port_opt) =>
                          (uri_opt, nodeId_opt, host_opt, port_opt) match {
                            case (Some(uri), None, None, None) => complete(eclairApi.connect(Left(uri)))
                            case (None, Some(nodeId), Some(host), _) => complete(eclairApi.connect(Left(NodeURI(nodeId, HostAndPort.fromParts(host, port_opt.getOrElse(NodeURI.DEFAULT_PORT))))))
                            case (None, Some(nodeId), None, None) => complete(eclairApi.connect(Right(nodeId)))
                            case _ => reject(MalformedFormFieldRejection("connect", "You must either provide field 'uri' or field 'nodeId' with optional fields 'host' and 'port'"))
                          }
                        }

This is much closer to what we have and there's only 1 or 2 places where we would need to do this. WDYT ?

[araspitzu]

I like the suggestion of not matching on multiple formFields if we can do that it's better than the workaround.

[pm47]

another option is just to avoid matching on multiple formField extractors and do something like:

It's less elegant than what we have now, but if that solves the issue and saves us from using timeouts, then I'm all for it.

[araspitzu]

withChannelIdentifier makes it quite hard to apply the change, especially in places like /close where we do match multiple formFields because of the channel identifier extraction that has been factored out. All the endpoints where we have

 withChannelIdentifier { channelIdentifier =>
   formField( fieldA, fieldB ) { 
      // complete
   }
 }

We're matching multiple formFields and it would become:

 formField(shortChannelId.?, channelId.? fieldA, fieldB ) { 
    case (Some(shortId), None, fieldA, fieldB) => // complete
    case (None, Some(channelId), fieldA, fieldB) => // complete
 }

[sstone]

And I missed that there is another form field for the API timeout....

[sstone]

LGTM, just one more change request

[sstone]

My understanding is that this is our input time-out i.e the time we'll wait the whole request to be received. Since our API request are really small this time-out should be just a few seconds, I suggest we set it to 5 seconds instead.
There should also be a comment that explains what this timeout is for, and the impact this change if we ever decide to add API calls with very large inputs (which is unlikely)

[araspitzu]

To be specific this timeout puts a limit on the time spent receiving data of a request parameter, ACK your suggestion I'll put a smaller timeout and a comment.

[pm47]

Looks ok, hope there won't be side effects !