[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

Mark Thomas  changed:

   What|Removed |Added

 Resolution|--- |FIXED
 Status|NEW |RESOLVED

--- Comment #19 from Mark Thomas  ---
Fixed in 9.0.x for 9.0.0.M18 onwards

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #18 from Mark Thomas  ---
Thanks for the offer.

I spend a little time looking at this over the weekend and I should have
something ready to commit shortly.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #17 from John D. Ament  ---
I'm fine with that as well.  Do you need me to make any changes to the patch?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #16 from romain.manni-bucau  ---
acceptable for me, thanks Mark

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #15 from Mark Thomas  ---
Given the concerns for existing applications - although I not convinced those
concerns are entirely valid - I propose that this change is implemented for
9.0.x only and an appropriate entry added to the migration guide.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #14 from Mark Thomas  ---
Why isn't the log4j2 issue just as much as a problem for an Executor thread
that is used to start multiple web applications?

I remain unconvinced that switching to the main thread when startStopThreads ==
1 is any worse that the current behaviour.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #13 from romain.manni-bucau  ---
side note: to speak about well known frameworks affected by that, log4j2 can
lead to the mentionned issue when added to the container since it sets a
threadlocal in initialzer or context listener and reset it in a servlet
(intended to be load on startup 1). Goal being to be initialized for the whole
startup duration but if another filter fails then it is not resetted. While
tomcat uses a pool this is not a big deal but when it will start to use the
caller thread it can fail where it was previously ok. See log4j-web for the
detail.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #12 from romain.manni-bucau  ---
Ok, if you have a listener observing before_init or before_start on Context (or
even org.apache.catalina.core.StandardHost#addChild) you can initialize some
context there, if you only deploy in this pool then you are safe to set and not
unset the value there.

I saw that several times in company integrations and I think it would be great
to not break them. Also keep in mind 1 is the default.

Regarding 0, you are right. For the story it means "auto number" from the
available processors which is something I missed.

Side note - unrelated to this ticket but still related to in context
deployment: for the embedded case using tomcat.getHost().addChild(context)
already deploys in current thread (that's what does Apache Meecrowave for
instance). This also means org.apache.catalina.core.ContainerBase#initInternal
could be lazy to avoid to create these useless threads.

Thinking more about it I think a clean solution would to just pass an executor
name like for connectors and just lookup the executor from the server.xml
definitions. If you don't want that pool deployment tomcat can provide an
InCallerThreadExecutor implementing Executor interface.

wdyt?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #11 from Mark Thomas  ---
You are going to have to explain that code sample. It means nothing to me.

0 and negative numbers are already defined to have special meaning.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #10 from romain.manni-bucau  ---
> Code that relies on the deployment mechanism for correct clean-up is broken 
> and needs to be fixed. Deployment threads may be re-used before they are 
> stopped and if any component fails to fully clean-up, the data could leak to 
> the next deployment and potentially cause a security issue.

No cause it is only in these threads so this pattern works well without
leaking:

deploymentStart() { context.set(new DeploymentEn(...)); }
deploymentStop() {}


Don't get me wrong, I don't like that code but saw it often and with tomcat
behavior it was more than valid.

> Re-defining a value of 1 for startStopThreads to mean 'serial deployment on 
> the main processing thread' rather than 'serial deployment via a thread pool 
> with a single thread' looks perfectly reasonable to me at this point.

I think I didn't get what was the issue using 0 for that? It keeps
compatibility and allow this new feature as well.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #9 from Mark Thomas  ---
Code that relies on the deployment mechanism for correct clean-up is broken and
needs to be fixed. Deployment threads may be re-used before they are stopped
and if any component fails to fully clean-up, the data could leak to the next
deployment and potentially cause a security issue.

Re-defining a value of 1 for startStopThreads to mean 'serial deployment on the
main processing thread' rather than 'serial deployment via a thread pool with a
single thread' looks perfectly reasonable to me at this point.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #8 from romain.manni-bucau  ---
Fact is tomcat kind of guarantee you execute code in a specific thread pool
(far from any user pool or http pool to be concrete).

So there is now a lot of code relying on deployment "initialization" setting
there and not cleaning it (for good or bad reasons depending the listener
implementation/hook). This is not an issue - assuming classes are coming from
the "container" - since you are limited to this pool so it will be initialized
each time and runtime will either not use that or get null and handle it
(that's the 2 cases I'm aware of).

Add to that #pool=1 is used to avoid concurrent deployments and uresing this
value to change the deployment mode will break instances.

Also just on a semantic point of view pool size = 1 sounds like a pool size is
1 and not 0 which correspond to "execute in context" no?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #7 from Mark Thomas  ---
(In reply to romain.manni-bucau from comment #4)
> Can 1 keep current behavior and 0 or negative values use an executor
> executing directly the task? 1 is used and relying on deployer threads
> avoids to leak data where this change would enable leakages for numerous
> cases like html deployer.

?

Please explain. The origins of the startStopThreads feature are not related to
memory leak prevention.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #6 from John D. Ament  ---
Just to clarify:

- If you don't specify the setting, or have it set to 0, you get the old
behavior.
- If you set it to 1, you get the new behavior.
- If you set it to anything larger than 1, you get the old behavior.

Romain does raise a good point about the negative number, I suspect you'll get
a runtime exception right now.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #5 from John D. Ament  ---
Romain, what would be the benefit of current behavior when threads == 1?  This
is specific to helping fix a problem w/ Weld and Tomcat integration where a
deadlock can occur because two threads are trying to gain a lock on the same
object.

I think one of the ideas that came up on the discussion was to just introduce a
flag. Mark seemed to think it was fine to just default to main.

Also note - the default behavior is that its going to use the thread count from
the processor to make a judgment on the number of executors to use.  Unless
you're explicitly overriding it to 1, you'll get the old behavior.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #4 from romain.manni-bucau  ---
Can 1 keep current behavior and 0 or negative values use an executor executing
directly the task? 1 is used and relying on deployer threads avoids to leak
data where this change would enable leakages for numerous cases like html
deployer.

Side note: in the PR stop needs to execute returned runnables i think if kept
this way

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #3 from John D. Ament  ---
I also created a PR not sure which would be easier.
https://github.com/apache/tomcat85/pull/6

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #2 from John D. Ament  ---
Created attachment 34660
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=34660=edit
Proposed code changes (based on dev discussion)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 60623] When startStopThreads is 1, don't rely on an executor and instead start synchronously

[bugzilla]

https://bz.apache.org/bugzilla/show_bug.cgi?id=60623

--- Comment #1 from Michael Osipov <1983-01...@gmx.net> ---
Alternatively, one can provide a #join() method just like Jetty does in
Server.class.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org