-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Run reactive rest client on Vertx same context #19225
Conversation
LGTM, but I would like @stuartwdouglas to take a look as well because he had opened #19144 |
We could make a PR with both sets of commits. |
The question is though, are they both needed? |
I'm not sure what Stuart was aiming to do, but I'm quite certain that we need the "run on context" that Stef introduced here. I would suggest to merge this one first and then ask Stuart if he wants to either drop his PR or rebase it on top. |
Yeah, I agree that this should be enough |
I thought so too, but I tried, and no: we get notified on the IO thread, so we will in turn notify on that same IO thread, which is different to the original worker thread. I'm not sure if we should aim to do that, though. This feels like a broader discussion, because if Vert.x doesn't do thread affinity with worker threads, there's probably a good reason? |
I wouldn't know TBH, but I guess :) |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building bf3fdd0
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
⚙️ JVM Tests - JDK 11 Windows #📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
⚙️ JVM Tests - JDK 16 #📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
|
Pretty sure the CI failures are related. |
So my PR basically makes sure that if we are dispatching back to the IO thread we pick the same IO thread that is tied to the connection, rather than a random IO thread. This mimics the server behavior. We have to be really careful with mutltiple threads being involved in reactive requests, there is a lot of potential for hard to debug races. |
@@ -202,7 +203,14 @@ public HttpClientResponse getVertxClientResponse() { | |||
@Override | |||
protected Executor getEventLoop() { | |||
if (httpClientRequest == null) { | |||
return restClient.getVertx().nettyEventLoopGroup().next(); | |||
// make sure we execute the client callbacks on the same context as the current thread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not right, if this is being called from a worker then it will dispatch on the worker. You still need to actually dispatch to the relevant IO thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can this be called from a worker thread, though? I've always seen it called from an IO thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be called by the user in org.jboss.resteasy.reactive.client.spi.ResteasyReactiveClientRequestContext from a org.jboss.resteasy.reactive.client.spi.ResteasyReactiveClientRequestFilter, which could be called from pretty much anywhere.
TBH I am really worried about this 'swapping threads' approach. It opens the door to lots of subtle bugs as we now need to ensure that we always do some kind of safe hand off between the threads. We also need to be super careful with IO, as things work differently if you are not on your IO thread, because as soon as you resume IO its possible another IO thread has started running the input handler, so you need to thing about the thread safety semantics of that.
From a performance point of view it also sucks, as it will involve multiple IO threads waking up selectors that don't belong to them, which is terrible from a performance POV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the concerns, there's many similar difficulties with Hibernate Reactive.
As a user, I think I'd be fine to have to deal with stronger restrictions in exchange for less [potential] trouble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just merge this for now (once the tests are passing), but look to the possibility of moving to a single thread model in future.
Thinking about it I am not sure how practical it will be, as having basically a pool of connections per IO thread is not going to be great for databases which have connection limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So can we merge this or not? Not trying to be pushy, but Hibernate Transactions are still broken without this (or a similar) fix :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should merge it for now, as I said above. I have rebased so lets see what CI says.
bf3fdd0
to
61d5785
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 61d5785
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 5 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 5 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
⚙️ JVM Tests - JDK 16 #- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment extensions/smallrye-reactive-messaging-kafka/deployment
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 7 more 📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖
✖
✖
📦 extensions/smallrye-reactive-messaging-kafka/deployment✖
|
Force-rebased, fixed the three tests. The kafka one on JDK16 I hope is unrelated. Let's see what CI says. |
I can confirm this fixes the issue I have in my cheese shop demo, where each REST client has its own IO thread, and I'm not sure what vert.x context they share. |
All checks passed, so it's up to @stuartwdouglas to reassess his review :) |
Hi, when will this be released? We want to go to production next week and would really like to have our ELK working by then. |
@FroMage @stuartwdouglas I am kinda hesitant to backport this, but if you think we should then I will. |
I think we should, it's pretty broken ATM. |
Okay |
Thank you - greatly appreciated. :) |
So likely to be included in |
I built the 2.2.3.Final with the changes from this PR locally, retested, and successfully verified that it should fix the problem. |
Fixes #18977