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

Avoid cyclic type reference #33

Closed
wants to merge 4 commits into from

Conversation

xerial
Copy link
Contributor

@xerial xerial commented Aug 19, 2016

This PR removes generic interfaces from Buffer, Sender and Flusher. This change does not break existing Fluency APIs, so no test (or user) code change is required.

Background
We are trying to use Fluency in a Scala project, but encountered a tricky error when evaluating Scala code that uses Fluency at runtime: wvlet/wvlet#8

[info]   scala.reflect.internal.Symbols$CyclicReference: illegal cyclic reference involving type C

This happens because there are complex cyclic references of generic types:

Buffer<T> { ...  Buffer.Config<T extends Buffer, C extends Config> }

Buffer references Config, which also checks parent Buffer and self Config type. This compiles in Java, but when using from scalac with reflective compilation (runtime eval of Scala code), the cyclic reference causes the above error.

More accurately, this type definition needs to be:

Buffer<T> { ... Buffer.Config<T extends Buffer<T>, C extends Config<T, C> }

But this adds the complexity to the code, so I simplified the interface structures by removing generics, and introduced ConfigImpl getConfig() to get Buffer/Sender/Flusher specific configurations.

@coveralls
Copy link

coveralls commented Aug 19, 2016

Coverage Status

Coverage decreased (-0.6%) to 86.224% when pulling 4dc7757 on xerial:avoid-cyclic-type into a215499 on komamitsu:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 86.224% when pulling 4dc7757 on xerial:avoid-cyclic-type into a215499 on komamitsu:master.

@xerial
Copy link
Contributor Author

xerial commented Aug 19, 2016

FYI: In general we usually have a better way to do the same thing instead of using circular reference: http://stackoverflow.com/questions/7186774/how-to-get-circular-generics-working-in-java

@komamitsu
Copy link
Owner

http://stackoverflow.com/questions/7186774/how-to-get-circular-generics-working-in-java

The code in this stackoverflow is invalid and can't be compiled in Java while org.komamitsu.fluency.buffer.Buffer is valid in Java. I don't get why it was introduced here...

Also, I confirmed the following simple Scala code can be compiled and works.

object Main {
  def main (args: Array[String]): Unit = {
    val fluency = org.komamitsu.fluency.Fluency.defaultFluency()
    val event = new java.util.HashMap[String, Object]
    event.put("k", "v")
    fluency.emit("foo.bar", event)
    fluency.close
    Thread.sleep(10000) // TODO: Use `fluency.isTerminated()`
  }
}

Could you share a minimum reproducible code?

@Lewuathe
Copy link

Lewuathe commented Aug 30, 2016

We have seen this case when we use airframe (at past wvlet-inject)

The case when cyclic reference is occurred is like below.

trait MyLog {
  val flog = inject[Fluency]
}

class SomeClass extends MyLog {
  // ...
}

val design = new SomeModule()
val session = design.newSession // <-- cyclic reference was occurred here. Resolving Fluency build graph failed.
val some = session.get[SomeClass] 

The latest airframe and patched fluency can fix this problem.

@xerial
Copy link
Contributor Author

xerial commented Aug 30, 2016

@komamitsu There is nothing wrong in fluency side as long as it is used in Java, which allows cyclic type references, but if you can simplify the current configuration type hierarchies, this benefits our Scala projects (e.g., https://github.com/wvlet/airframe)

I agree in that generics is useful to avoid downcasts, but these configuration objects are quite simple ones, so splitting config objects into separate classes can be alternative approach. For example, if we have separate SenderConfig and TCPSenderConfig (only contains TCP related configurations) rather than extending SenderConfig, we don't need to use Generics.

@xerial
Copy link
Contributor Author

xerial commented Aug 30, 2016

@komamitsu An only bad thing in the current generic usage is it involves abstract types:

Buffer<T> { ...  
   Buffer.Config
    <T extends Buffer,   <<---   This Buffer type is an abstract type. Buffer<T> is correct
     C extends Config>  <<---   This Config type is also an abstract type. Config<T, C> is correct  
}

Then when (dynamically) compiling this code in Scala, this causes:

[info]   scala.reflect.internal.Symbols$CyclicReference: illegal cyclic reference involving type C

I highly recommend to avoid using generics to implement such a simple configuration hierarchy. If you want, I can create another PR that completely removes generics.

@komamitsu komamitsu mentioned this pull request Sep 11, 2016
@komamitsu
Copy link
Owner

@xerial @Lewuathe I released fluency 1.0.0 and I confirmed the unit tests of wvlet's fluency-injection-test branch finished successfully with the new version.

@xerial
Copy link
Contributor Author

xerial commented Sep 20, 2016

Thanks for the fix! I'll close this ticket :)

@xerial xerial closed this Sep 20, 2016
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

Successfully merging this pull request may close these issues.

4 participants