actions-on-google / actions-on-google-java

Java/Kotlin library for Actions on Google
Apache License 2.0
286 stars 39 forks source link

SmartHomeApp.reportState error #30

Open kingskylar opened 5 years ago

kingskylar commented 5 years ago

Is this error below because the grpc-core version is too low?

io.grpc.internal.ManagedChannelOrphanWrapper line:163 -*~*~*~ Channel ManagedChannelImpl{logId=144961, target=homegraph.googleapis.com} was not shutdown properly!!! ~*~*~*
    Make sure to call shutdown()/shutdownNow() and wait until awaitTermination() returns true.
java.lang.RuntimeException: ManagedChannel allocation site
    at io.grpc.internal.ManagedChannelOrphanWrapper$ManagedChannelReference.<init>(ManagedChannelOrphanWrapper.java:103) [grpc-core-1.15.1.jar:1.15.1]
    at io.grpc.internal.ManagedChannelOrphanWrapper.<init>(ManagedChannelOrphanWrapper.java:53) [grpc-core-1.15.1.jar:1.15.1]
    at io.grpc.internal.ManagedChannelOrphanWrapper.<init>(ManagedChannelOrphanWrapper.java:44) [grpc-core-1.15.1.jar:1.15.1]
    at io.grpc.internal.AbstractManagedChannelImplBuilder.build(AbstractManagedChannelImplBuilder.java:410) [grpc-core-1.15.1.jar:1.15.1]
    at com.google.actions.api.smarthome.SmartHomeApp.reportState(SmartHomeApp.kt:126) [actions-on-google-1.5.0.jar:?] 
cbeaujoin commented 5 years ago

No, this means that the channel to Dialogflow API is not closed programmatically. To safely close the channel as it is said : "Make sure to call shutdown()/shutdownNow() and wait until awaitTermination() returns true."

kingskylar commented 5 years ago

Thank you for reply! I use the method in actions-on-google-1.5.0.jar to report states to google.Is there a problem with this method?

/**
     * Sends a ReportState command to the Home Graph, which will store a device's current state.
     * This should be called after a device receives an EXECUTE request, or if the device has
     * changed state through a means outside of your smart home Action.
     *
     * @param request A payload containing a series of devices and their connected states
     * @return A response to the API call
     */
    fun reportState(request: HomeGraphApiServiceProto.ReportStateAndNotificationRequest):
            HomeGraphApiServiceProto.ReportStateAndNotificationResponse {
        if (this.credentials == null) {
            throw IllegalArgumentException("You must pass credentials in the app constructor")
        }
        val channel = ManagedChannelBuilder.forTarget("homegraph.googleapis.com").build()

        val blockingStub = HomeGraphApiServiceGrpc.newBlockingStub(channel)
                // See https://grpc.io/docs/guides/auth.html#authenticate-with-google-3.
                .withCallCredentials(MoreCallCredentials.from(this.credentials))

        return blockingStub.reportStateAndNotification(request)

    }
cbeaujoin commented 5 years ago

You could try something like this :

/**
     * Sends a ReportState command to the Home Graph, which will store a device's current state.
     * This should be called after a device receives an EXECUTE request, or if the device has
     * changed state through a means outside of your smart home Action.
     *
     * @param request A payload containing a series of devices and their connected states
     * @return A response to the API call
     */
    fun reportState(request: HomeGraphApiServiceProto.ReportStateAndNotificationRequest):
            HomeGraphApiServiceProto.ReportStateAndNotificationResponse {
        if (this.credentials == null) {
            throw IllegalArgumentException("You must pass credentials in the app constructor")
        }
        val channel = ManagedChannelBuilder.forTarget("homegraph.googleapis.com").build()

        val blockingStub = HomeGraphApiServiceGrpc.newBlockingStub(channel)
                // See https://grpc.io/docs/guides/auth.html#authenticate-with-google-3.
                .withCallCredentials(MoreCallCredentials.from(this.credentials))

        ListenableFuture<HomeGraphApiServiceProto.ReportStateAndNotificationResponse> response = blockingStub.reportStateAndNotification(request);
        channel.shutdown();
        try {
            channel.awaitTermination(500, TimeUnit.MILLISECONDS);
        } catch (InterruptedException ex) {
            ex.printStackTrace();
        }
        return response;
    }
Fleker commented 5 years ago

I've not seen that error before. Has it only begun happening recently? How are you hosting the code?

kingskylar commented 5 years ago

This error has been around, causing my application to die because of increased memory.I just used the method SmartHomeApp.reportState in actions-on-google-1.5.0.jar,May be caused by a network connection problem If the network is too slow or not connected.

Fleker commented 5 years ago

How are you hosting the code? Google App Engine? Some other provider like AWS?

kingskylar commented 5 years ago

AWS

Fleker commented 5 years ago

Can you try the suggestion that @cbeaujoin created? Maybe the channel needs to be shutdown after use, or instantiated once in the run environment instead of being continually created?

I've not seen any memory issues, so this may be useful for performance improvements.

RayBa82 commented 5 years ago

I have the same error when using report state. How should i try the suggestions? Changing the code and building the project?

Fleker commented 5 years ago

Does shutting down the channel work after it is used?

RayBa82 commented 5 years ago

I dont know what you are talking about? I am using just SmartHomeApp.reportState. There is no method to shut down a "channel"

RayBa82 commented 5 years ago

BTW I have a working implementation for node, i just converted this app to java because node sucks ;-) And now I get this error. So the error has to be in the java implementation.

After having done all the work i am very frustrated to see that the java implementation has so much errors. This all feels like an early alpha version. I expect better from a company like google.

Switching to Alexa becomes more and more attractive, not only because of the much wider supported device range, the development happens so much faster over there.

Fleker commented 5 years ago

@RayBa82 if you look at the source code for the library, you will see the underlying gRPC implementation.

Are you also using AWS for hosting your project?

RayBa82 commented 5 years ago

I am hosting it on my own server. I have zero experience with gRPC, so I would have to dig into it. Maybe using kotlin with node is easyer....

Fleker commented 5 years ago

Can you try copying the suggested function @cbeaujoin has and calling that instead of the current function?

RayBa82 commented 5 years ago

Ok now i got it, I will try that tonight CET ;-)

Fleker commented 5 years ago

Thanks. Let me know. I have not experienced any issues running on Google App Engine, so it'll be interesting to see if the behavior exists in other instances.

RayBa82 commented 5 years ago

This really seems to fix the problem. I could not reproduce the error with the fix. Thanks.

RayBa82 commented 5 years ago

This code works without any problem: ' fun report(request: HomeGraphApiServiceProto.ReportStateAndNotificationRequest): HomeGraphApiServiceProto.ReportStateAndNotificationResponse { requireNotNull(this.credentials) { "You must pass credentials in the app constructor" } val channel = ManagedChannelBuilder.forTarget("homegraph.googleapis.com").build() val blockingStub = HomeGraphApiServiceGrpc.newBlockingStub(channel) // See https://grpc.io/docs/guides/auth.html#authenticate-with-google-3. .withCallCredentials(MoreCallCredentials.from(this.credentials)) val response = blockingStub.reportStateAndNotification(request) channel.shutdown() return response } '

waiting for termination seems not to be necessary

EDIT: Without waiting for termination the error occured sometimes again, so better not remove it :-)

RayBa82 commented 5 years ago

Just for further Info:

This error never happened with a single request. But if my applications was sending multiple report state requests per second (about 6) this error has always been reproducable.

Maybe you can reproduce this with a load test in App Engine too.

cbeaujoin commented 5 years ago

Hi,

I had the same problem using the google-cloud-dialogflow lib and I don't exactly remind the cause. But it seems Google App Engine internally manage grpc channels. When deploying to an other environment, the ExecutorService has to be manage manually as explain here : google-cloud-java/issues/4211

Fleker commented 5 years ago

@proppy can you assist with this issue?

kingskylar commented 4 years ago

@Fleker How is this issue? Because of this issue, I have to turn off the report function.For authentication i have to use this function now.Please fix it soon,thank you!

RayBa82 commented 4 years ago

None of the solutions with manually closing the channel worked for me in the long run. After some time the error always occured again.

What actually worked for me is making val channel: ManagedChannel = ManagedChannelBuilder.forTarget("homegraph.googleapis.com").build() a class field with direct initialization.

But i have to say im running a spring boot application and my SmartHomeApp class is a spring bean. So i only have one instance of the class for my whole application.

But this is running stable without any error over weeks.

Fleker commented 4 years ago

Where are you hosting it?

RayBa82 commented 4 years ago

It runs on a virtual private server as docker container.

kingskylar commented 4 years ago

As long as i enable the report function,lots of the error happend,below is a log screenshot.

error
HuangDayu commented 4 years ago

Hello, I am using version 1.8 and also encountering this problem, how can I solve it? Thank you.

2020-05-19 10:28:48.521 ERROR 19974 --- [http-nio-8016-exec-17] i.g.i.ManagedChannelOrphanWrapper        : *~*~*~ Channel ManagedChannelImpl{logId=4349, target=homegraph.googleapis.com} was not shutdown properly!!! ~*~*~*
    Make sure to call shutdown()/shutdownNow() and wait until awaitTermination() returns true.

java.lang.RuntimeException: ManagedChannel allocation site
        at io.grpc.internal.ManagedChannelOrphanWrapper$ManagedChannelReference.<init>(ManagedChannelOrphanWrapper.java:103) ~[grpc-core-1.15.1.jar:1.15.1]
        at io.grpc.internal.ManagedChannelOrphanWrapper.<init>(ManagedChannelOrphanWrapper.java:53) ~[grpc-core-1.15.1.jar:1.15.1]
        at io.grpc.internal.ManagedChannelOrphanWrapper.<init>(ManagedChannelOrphanWrapper.java:44) ~[grpc-core-1.15.1.jar:1.15.1]
        at io.grpc.internal.AbstractManagedChannelImplBuilder.build(AbstractManagedChannelImplBuilder.java:410) ~[grpc-core-1.15.1.jar:1.15.1]
        at com.google.actions.api.smarthome.SmartHomeApp.reportState(SmartHomeApp.kt:126) ~[actions-on-google-1.8.0.jar:na]
        ....
murilobaliego commented 4 years ago

Hi @Fleker what is the solution for this error? I'm getting the same error with latest version 1.8 and hosting in google cloud: 2020-06-12 13:31:50.798 BRT io.grpc.internal.ManagedChannelOrphanWrapper$ManagedChannelReference cleanQueue: ~~~ Channel ManagedChannelImpl{logId=70, target=homegraph.googleapis.com} was not shutdown properly!!! ~~~ (ManagedChannelOrphanWrapper.java:151) Make sure to call shutdown()/shutdownNow() and wait until awaitTermination() returns true.

Fleker commented 4 years ago

@proppy can you provide some additional guidance on this issue?

proppy commented 4 years ago

Sorry for not seeing the @mention earlier.

It seems that action-on-google-java creates a new ManagedChannel on every request: https://github.com/actions-on-google/actions-on-google-java/blob/master/src/main/kotlin/com/google/actions/api/smarthome/SmartHomeApp.kt#L126

As pointed by @cbeaujoin in https://github.com/actions-on-google/actions-on-google-java/issues/30#issuecomment-529400776 and comments in the official gRPC sample for Java: https://github.com/grpc/grpc-java/blob/master/examples/src/main/java/io/grpc/examples/helloworld/HelloWorldClient.java#L96 such channels should be explicitly shutdown when no longer being used to prevent leaks.

A workaround could be for client to directly depends on the HomeGraph API Client Library for Java https://github.com/googleapis/google-api-java-client-services/tree/master/clients/google-api-services-homegraph/v1 instead of using the reportState helper and reuse a single channel tied to the lifetime of the application.

kingskylar commented 4 years ago

@Fleker Hi,how is this issue?it's a long time.

fangquan456 commented 1 year ago

@proppy proppy can you supply a demo?(A workaround could be for client to directly depends on the HomeGraph API Client Library for Java )

wuhaoisme commented 3 months ago

actionsApp.reportState(request);

change as

HomeGraphApiServiceGrpc.HomeGraphApiServiceBlockingStub stub = HomeGraphApiServiceGrpc.newBlockingStub(channel) .withCallCredentials(MoreCallCredentials.from(actionsApp.getCredentials())); stub.reportStateAndNotification(request);

finally releaseChannel(channel)