Implement zealous block detection for HttpClient #10010
Conversation
This new check throws an exception if *any* event loop thread would be blocked by a call to the netty BlockingHttpClient. This is usually a bug, as a blocked event loop can lose data. However there are legitimate cases where this is fine, so there's also a config option to disable this detection. But all the cases in the micronaut-core test suite where this made a test fail were unintentional and potentially buggy, so most users should leave the setting as-is. The error message provides two code remediation options. It also mentions that there's a config property for this, but doesn't explain which, because I want to discourage an "easy fix" that will leave the bug in place.
http-client-core/src/main/java/io/micronaut/http/client/HttpClientConfiguration.java
Show resolved
Hide resolved
http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java
Outdated
Show resolved
Hide resolved
http-client-core/src/main/java/io/micronaut/http/client/HttpClientConfiguration.java
Outdated
Show resolved
Hide resolved
| loop thread. This is a common cause for bugs: Event loops should \ | ||
| never be blocked. You can either mark your controller as \ | ||
| @ExecuteOn(TaskExecutors.BLOCKING), or use the reactive HTTP client \ | ||
| to resolve this bug. There is also a configuration option to \ |
There was a problem hiding this comment.
| to resolve this bug. There is also a configuration option to \ | |
| to resolve this bug. There is also a configuration option micronaut.http.client.allow-block-event-loop=true to \ |
It also mentions that there's a config property for this, but doesn't explain which, because I want to discourage an "easy fix" that will leave the bug in place.
I don't agree with this. I think we should say which is the config property.
There was a problem hiding this comment.
people will just enable the setting and move on without fixing their bug
There was a problem hiding this comment.
I understand you are trying to encourage correct behavior.
However, we should not have secret properties they have to seek. If a setting exists, we have to document it. Let's say it is discouraged, and we can warn users about user it. But, we cannot expect our users to clone micronaut core and dive into our codebase to understand what is available.
There was a problem hiding this comment.
Why not linking to a place in the docs where we explain why this is bad, and also document the property in a warning box?
# Conflicts: # http-client/src/test/groovy/io/micronaut/http/client/BlockingSpec.groovy
solved the merge. |
|
Please retry analysis of this Pull-Request directly on SonarCloud. |
This new check throws an exception if any event loop thread would be blocked by a call to the netty BlockingHttpClient. This is usually a bug, as a blocked event loop can lose data.
However there are legitimate cases where this is fine, so there's also a config option to disable this detection. But all the cases in the micronaut-core test suite where this made a test fail were unintentional and potentially buggy, so most users should leave the setting as-is.
The error message provides two code remediation options. It also mentions that there's a config property for this, but doesn't explain which, because I want to discourage an "easy fix" that will leave the bug in place.