-
Notifications
You must be signed in to change notification settings - Fork 422
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
Move task cancelation on blocking pool #1851
Conversation
@@ -26,7 +26,7 @@ object VertxZioServerOptions { | |||
CustomInterceptors( | |||
createOptions = (ci: CustomInterceptors[RIO[R, *], VertxZioServerOptions[RIO[R, *]]]) => | |||
VertxZioServerOptions( | |||
Defaults.createTempFile().getParentFile.getAbsoluteFile, | |||
new java.io.File(System.getProperty("java.io.tmpdir")).getAbsoluteFile, |
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 noted that new temp file is created each time when server is started. It seems that it is equivalent code to get temp dir.
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.
makes sense, thanks :)
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.
actually the same code is present in cats & future interpreters, so maybe we could make ew java.io.File(System.getProperty("java.io.tmpdir"))
an internal utility function and simply use it in these places
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.
OK. I moved this line inside function VertxServerOptions.uploadDirectory()
@@ -75,14 +75,20 @@ trait VertxZioServerInterpreter[R] extends CommonServerInterpreter { | |||
|
|||
rc.response.exceptionHandler { (t: Throwable) => | |||
cancelRef.getAndSet(Some(Left(t))).collect { case Right(c) => | |||
c(FiberId.None) | |||
rc.vertx().executeBlocking[Unit]((promise: Promise[Unit]) => { |
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.
Thanks! Shouldn't we do the same in VertxCatsServerInterpreter
?
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 looks like this issue is not applied for VertxCatsServerInterpreter
because CancelToken
is function from Unit
to Future[Unit]
. So calling such function doesn't block caller and just returns Future
.
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.
Ah ok:)
2475f41
to
9e8f68a
Compare
Thanks! |
Now task cancelation is executed on main vertx pool. But task cancelation may be blocking operation. Here is such example:
So interrupting of several tasks may block main vertx pool and leads to following warnings
So it is better to execute this operation on blocking pool.