bug fix + ci / sbt / scala upgrades

[ahjohannessen]

• adjust .travis.yml to separate compilation from
  test in order to have more memory when running
  tests.

• change NettyConnectionPool to remove ClientNegotiateCapabilities
  when validation of capabilities fails in order to avoid having it
  try parsing an empty string just before failing with IncompatibleServer.

  Moreover, instruct sbt to use more memory
  for heap and stack.

• bump dependencies scodec-core to 1.8.2,
  scalaz-stream to 0.7.3a and scalaz-core
  to 7.1.3.

• bump version of scala to 2.11.7 and 2.10.6,
  and sbt to 0.13.9.

[ahjohannessen]

@timperrett I get the same exception as in #93 - It only happens when using oraclejdk8 - I see the same error on shippable, so I suppose we have some sort of a bug. Here is the failure:

[info] - should not call an incompatible server *** FAILED ***
[info]   Expected exception remotely.transport.netty.IncompatibleServer to be thrown, but java.lang.IllegalArgumentException was thrown. (CapabilitiesSpec.scala:45)

with this exception (note that it is an empty string it cannot parse):

java.lang.IllegalArgumentException: could not parse greeting from server: 

thrown here: ClientConnectionPool.scala:221.

Odd thing being is that shippable with oraclejdk8 passes with 2.11.7, but fails with 2.10.5 and travis does the opposite. The JDK versions are a little bit different:

shippable:

java version "1.8.0_25"
Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)

travis:

java version "1.8.0_31"
Java(TM) SE Runtime Environment (build 1.8.0_31-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.31-b07, mixed mode)

[timperrett]

Agreed - it looks like a bug. Hmmmmm. What would you like to do here? Is it passing locally for you?

[ahjohannessen]

I'll try to reproduce with same setup on a linux machine locally when I find the time. It passes locally, but I am on OSX :)

[ahjohannessen]

I have tried to run tests with scala 2.11.7 / 2.10.5 with same JDK version as Travis on Ubuntu 15.04 and all tests pass. A bit lost here.

[ahjohannessen]

@timperrett @stew I have done a little bit of debugging and I think I have found the error. Did the following in order to isolate what is messing things up:

// Capabilities.scala

  def parseHelloString(str: String): Attempt[Capabilities] = {
    val debugParser = log(helloString)("Capabilities Parser")
    val pr = parseAll(debugParser, str)
    if(pr.successful) Attempt.successful(pr.get)
    else Attempt.failure(Err("could not parse greeting from server: " + str))
  }

Getting this output (both on my OS X and Linux machines):

ClientNegotiateCapabilities.decode with input: OK: []

trying Capabilities Parser at scala.util.parsing.input.CharSequenceReader@f4ea326
Capabilities Parser --> [1.7] parsed: Capabilities(Set())
validateCapabilities missing: Set(Remotely 1.0)
ClientNegotiateCapabilities.decode with input:
trying Capabilities Parser at scala.util.parsing.input.CharSequenceReader@264a28fe
Capabilities Parser --> [1.1] failure: `OK: ' expected but end of source found

^

That made me focus on what happens in ClientNegotiateCapabilities.

// ClientConnectionPool.scala

  val validateCapabilities: ((Capabilities,Channel)) => Task[Channel] = {
    case (capabilties, channel) =>
      val missing = Capabilities.required -- capabilties.capabilities
      if(missing.isEmpty) {
        Task {
          val pipe = channel.pipeline()
          pipe.removeLast()
          pipe.addLast("enframe", Enframe)
          pipe.addLast("deframe", new Deframe)
          channel
        }
      }
      else {
        channel.close()
        Task.fail(IncompatibleServer("server missing required capabilities: " + missing))
      }
  }

I realized that pipe.removeLast() was to remove to negotiateCapable for happy path. However, channel.close causes negotiateCapable to be run and will result in having the
ChannelOutboundHandler#close(ChannelHandlerContext, ChannelPromise) method called of the next ChannelOutboundHandler contained in the ChannelPipeline of the Channel.

If I do this:

      else {
        channel.pipeline().removeLast()
        channel.close()
        Task.fail(IncompatibleServer("server missing required capabilities: " + missing))
      }

we do not end up invoking parseHelloString with an empty string.

[ahjohannessen]

Furthermore, perhaps it would be better to register handlers with name and remove them by name using

addLast(String name, ChannelHandler handler);
ChannelHandler remove(String name);

[ahjohannessen]

My guess for CapabilitiesSpec being flaky is because of shutdown races and CI machines being slower than local dev boxes.

@timperrett I am not sure that my fix is sound, I think @stew and you can make that call. However, as you see this is making Travis happy. My shippable CI setup of remotely is also happy :)

[ahjohannessen]

@jedesah channel.close bothers me a bit as it returns a ChannelFuture that is not handled in the same way as:

        Task.async[Channel] { cb =>
          val _ = fut.addListener(new ChannelFutureListener {
                     def operationComplete(cf: ChannelFuture): Unit = {
                        if(cf.isSuccess) cb(\/-(cf.channel)) else cb(-\/(cf.cause))
                      }
                  })
        }

at https://github.com/ahjohannessen/remotely/blob/wip-improve-ci-bump-versions/core/src/main/scala/transport/netty/ClientConnectionPool.scala#L258

[ahjohannessen]

If that would be included then I suppose it could be done like this:

        Task.async[Channel] { cb =>
          pipe.removeLast()
          val listener = new ChannelFutureListener {
             def operationComplete(cf: ChannelFuture): Unit = {
               if(cf.isSuccess) cb(\/-(cf.channel)) else cb(-\/(cf.cause))
             }
          }
          val _ = channel.close().addListener(listener)
        } flatMap { case _ => Task.fail(IncompatibleServer("...")) }

However, I am a bit ignorant when it comes to the netty stuff and whether a simple channel.close is ok in this scenario.