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

Use traits for SSL and Common TCP Stuff #12

Closed
purplefox opened this issue Jun 30, 2013 · 22 comments
Closed

Use traits for SSL and Common TCP Stuff #12

purplefox opened this issue Jun 30, 2013 · 22 comments
Milestone

Comments

@purplefox
Copy link
Member

Traits can be used for the common SSL and TCP stuff that's used by NetServer/NetClient and HttpServer/HttpClient to reduce unnecessary code duplication.

See TCPSupport/SSLSupport in the Java API for the equivalent in Java.

@swilliams-pivotal
Copy link
Contributor

This comment made me revisit the recent PR. I had implemented ReadStream & WriteStream as traits, but they have been removed from the classes they were applied to.

@purplefox
Copy link
Member Author

ReadStream and WriteStream should be traits too, but TCPSupport and SSLSupport are used for applying the standard TCP and SSL properties which is somewhat different.

@swilliams-pivotal
Copy link
Contributor

I am vaguely remembering having a problem implementing the traits with actual code (rather than just the interface), because a trait can't have a constructor parameter - meaning we can't delegate to the internal mechanism. (Happy to be proved wrong here, of course).

I attempted to create a marker trait for those classes which wrap a o.v.j.core class, which exposed the delegate via a protected method (so the real traits could depend on that), but ended up down a rabbit hole. Maybe I'll try again.

@raniejade
Copy link
Contributor

For scala classes which wraps a o.v.j.core class, we could use something like below:

private[core] abstract class ApiShim[T](private val internal: T) {
  def unwrap: T = internal
}

@swilliams-pivotal
Copy link
Contributor

How would this be applied to the ReadStream trait to enable its methods to contain actual code, rather than just method signature declarations? I couldn't figure that out.

@raniejade
Copy link
Contributor

Probably something like this?

abstract class ApiShim[T](private val internal: T) {
  def unwrap: T = internal
}

trait ReadStream[T <: org.vertx.java.core.streams.ReadStream[T]] { self: ApiShim[T] =>

  import org.vertx.scala.core.FunctionConverters._

  def dataHandler(handler: (Buffer) => Unit) = {
    unwrap.dataHandler(handler)
    this
  }

  def endHandler(handler: () => Unit) = {
    unwrap.endHandler(handler)
    this
  }

  def exceptionHandler(handler: Handler[Throwable]) = {
    unwrap.exceptionHandler(handler)
  }

  def pause() = {
    unwrap.pause()
    this
  }

  def resume() = {
    unwrap.resume()
    this
  }

}

class NetSocket(val internal: JNetSocket) extends ApiShim[JNetSocket](internal) with org.vertx.scala.core.streams.ReadStream[JNetSocket] {

}

@swilliams-pivotal
Copy link
Contributor

Awesome. The 'self' thing is what was eluding me.

I tweaked the stream traits earlier; I'll have a go at applying this approach.

@swilliams-pivotal
Copy link
Contributor

I've given it a try in a branch; but this doesn't compile for reasons that escape me.

https://github.com/vert-x/mod-lang-scala/blob/delegating_traits/src/main/scala/org/vertx/scala/core/streams/ReadStream.scala

@raniejade
Copy link
Contributor

I have fixed the compile errors, but there is a test failure. You could check https://github.com/raniejade/mod-lang-scala/tree/delegating_traits.

@raniejade
Copy link
Contributor

Sorry the test failure was because of a dirty build. Did a clean build and everything passed.

@edgarchan
Copy link
Contributor

i think due to the self type it's now possible use a trait, the abstract member will be resolved or injected until the concrete class construction ... incidentally we get rid of the noisy constructor and/or indirect calls.

https://gist.github.com/edgarchan/5905937

@swilliams-pivotal
Copy link
Contributor

OK, all the above works for me.

On 2 July 2013 05:36, Edgar Chan notifications@github.com wrote:

i think due to the self type it's now possible use a trait, the abstract
member will be resolved or injected until the concrete class construction
... incidentally we get rid of the noisy constructor and/or indirect calls.

https://gist.github.com/edgarchan/5905937


Reply to this email directly or view it on GitHubhttps://github.com//issues/12#issuecomment-20326008
.

@swilliams-pivotal
Copy link
Contributor

I updated the branch, but the signature of Pump.scala newPump() doesn't seem right & the associated file test doesn't compile.

@swilliams-pivotal
Copy link
Contributor

I'm reading this: http://www.artima.com/weblogs/viewpost.jsp?thread=270195 and wondering if we should be using a type statement.

@raniejade
Copy link
Contributor

Hmm.. That's interesting, gonna check it out! Thanks!. I somehow fixed the compile error, check this commit: raniejade@3ee2cd8

@edgarchan
Copy link
Contributor

great .. don't you think it's better to use the shorter version in this case .. ReadStream[_] instead of ReadStream[T] forSome {type T}

@swilliams-pivotal
Copy link
Contributor

Forgive my ignorance but are they exactly the same?

@edgarchan
Copy link
Contributor

i can't find a proper reference but look at this presentation, the relevant part is the Existential Types .. it can be used for sure.

slideshare.net/dgalichet/demystifying-scala-type-system

@raniejade
Copy link
Contributor

Yeah, ReadStream[_] and ReadStream[T] forSome {type T} are the same :).

@galderz
Copy link
Contributor

galderz commented Jul 26, 2013

@raniejade decided to take this on.

@raniejade
Copy link
Contributor

@swilliams-vmw The link you have provided looks promising, I will try to take look at it further.

@Narigo
Copy link
Member

Narigo commented Oct 18, 2013

This should be done with #63 too.

@Narigo Narigo closed this as completed Oct 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants