search

marcoferrer / kroto-plus

gRPC Kotlin Coroutines, Protobuf DSL, Scripting for Protoc
Apache License 2.0
493 stars 28 forks source link

Kotlin 1.3 and stable coroutines #4

Closed rocketraman closed 5 years ago

rocketraman commented 5 years ago

Any plans to update to Kotlin 1.3 and stable coroutines?

tim-fdc commented 5 years ago

Hi, also highly interested Kotlin 1.3 and stable coroutines support. Any plans? Thanks a lot!

marcoferrer commented 5 years ago

Absolutely! Sorry about the late reply, but yes I'm in the process of wrapping up the updates needed for supporting the new structured concurrency model and bumping all the dependency versions. Ill try to get the snapshot updated in the mean time so users can try it out before the official release. I should have everything done in time for the kotlin 1.3 / coroutines 1.0

Ive been splitting up my time between this and porting over the coroutine support to the official kotlin coroutines repo. So the plan one day will be to deprecate the coroutines support in this repo in favor of the official Kotlin integration.

tim-fdc commented 5 years ago

Perfect! Thanks Marco.

marcoferrer commented 5 years ago

Been really busy lately but finally got the snapshot for stable coroutines published. Im in the middle of cleaning up the readme and documenting some changes but you can try 0.2.0-SNAPSHOT ahead of the release if you'd like. Its in the following repo. 

repositories {
    maven { url 'https://oss.jfrog.org/artifactory/oss-snapshot-local' }
}

Im aiming to wrap up and release by monday morning.

essh commented 5 years ago

I gave this a whirl and it seems to be working fine in some limited testing.

The only thing I noticed is the following line: https://github.com/marcoferrer/kroto-plus/blob/6a8630eef0dff06eb924d4871c8ae28bdd4eab4d/kroto-plus-coroutines/src/main/kotlin/com/github/marcoferrer/krotoplus/coroutines/GrpcContextElement.kt#L22

The gRPC Context docs state the following (https://grpc.io/grpc-java/javadoc/io/grpc/Context.html#detach-io.grpc.Context-):

It is expected that between any pair of matching attach() and detach(), all attach()es and detach()es are called in matching pairs. If this method finds that this context is not current, either you or some code in-between are not detaching correctly, and a SEVERE message will be logged but the context to attach will still be bound. Never use Context.current().detach(), as this will compromise this error-detecting mechanism.

I have an older implementation of a gRPC ContextElement that does the equivalent of this@GrpcContextElement.context.detach(oldState) instead and it seems to work fine but I have not tested it in great depth.

marcoferrer commented 5 years ago

You bring up a good point. My reasoning behind using Context.current().detach(oldstate) was an effort to try to prevent an unexpected context state. Since users could potentially perform context modifications in a coroutine without properly managing attachs/detachs. In hind sight, the grpc context itself has mechanisms in place for warning about this sort of thing. Ill make sure to get the bug addressed.

Thanks for the heads up @essh