Make sure request resumes on correct thread #10176
Conversation
|
Your PR failed because of a code formatting failure which was pushed to master (weird that the PR checks didn't catch them). I fixed in #10175 |
|
Thanks, now please rebase this PR ;) |
|
It seems like |
|
@stuartwdouglas this change works around the issue with |
|
This looks like something we really want in CR1. |
Yes please !! |
|
This will also fix #10452 |
|
I just rebased that one. Stuart, where are we on this one? AFAICS the tests were failing and given the error message, I think it's related. |
|
@gsmet there's an ongoing discussion on Zulip about what's the proper fix for the issue. In a few words, the changes in this PR fix the symptom observed in #9693 but not the cause. |
|
I think I have a fix for these failing tests, but we also need to look at preserving the vert.x instance. This will be a seperate PR though, as this is more complex. |
|
I have added another commit that will limit us to a single vert.x, lets see what CI says |
|
@gsmet should be good to go now. |
| @@ -39,7 +40,8 @@ void handleHotReplacementRequest(RoutingContext routingContext) { | |||
| routingContext.next(); | |||
| return; | |||
| } | |||
| routingContext.vertx().executeBlocking(new Handler<Promise<Boolean>>() { | |||
| ConnectionBase connectionBase = (ConnectionBase) routingContext.request().connection(); | |||
There was a problem hiding this comment.
This change shouldn't be needed anymore.
There was a problem hiding this comment.
It is, the old code could resume on a different IO thread in the same vert.x instance, this makes sure it is the same thread that will be delivering the IO notifications.
There was a problem hiding this comment.
Sorry I can't see how, do you have a simple example in mind?
There was a problem hiding this comment.
Actually the original code should be safe now, as it stores the original context. Either way though this code is fine.
| .then().statusCode(200) | ||
| .body(Matchers.equalTo("test route")); | ||
|
|
||
| runner.modifySourceFile(TestRoute.class, new Function<String, String>() { |
There was a problem hiding this comment.
How about updating TestRoute so that the handler calls executing blocking, and we verify that the context when calling executingBlocking and when the callback is involved is the same:
Vertx vertx = rc.vertx()
expected = vertx.getOrCreateContext();
vertx.executeBlocking(Promise::complete, ar -> {
actual = vertx.getOrCreateContext();
// expected should be the same as actual
// before AND after hot reload
})There was a problem hiding this comment.
I have updated the test to verify the connection vertx instance matches the current vertx instance.
geoand
added
the
triage/waiting-for-ci
Fixes #9693