Re: (tomcat) branch 10.1.x updated: Fix backport of BZ 66508 regression fix
On 19/01/2024 10:24, Rémy Maucherat wrote: On Fri, Jan 19, 2024 at 11:08 AM Mark Thomas wrote: On 19/01/2024 09:22, Rémy Maucherat wrote: On Thu, Jan 18, 2024 at 8:18 PM wrote: This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git The following commit(s) were added to refs/heads/10.1.x by this push: new 67638c17e7 Fix backport of BZ 66508 regression fix 67638c17e7 is described below commit 67638c17e7c19a0280ccafa340183fb179af92f5 Author: Mark Thomas AuthorDate: Thu Jan 18 18:52:30 2024 + Fix backport of BZ 66508 regression fix I don't understand the differences introduced. In SocketWrapperBase, the lock is not a ReentrantLock, but it still cannot be anything else: private final Lock lock = new ReentrantLock(); So couldn't the code be the same as in Tomcat 11 ? (with an extra cast, but no need to check anything) SocketWrapperBase.getLock() isn't final so it is possible for the SocketWrapper implementation in a sub-class to override it and use a different lock type. I think ReentrantLock is the only realistic option here but I was aiming to be extra careful to avoid breakage. We do have some users that implement custom connectors so I couldn't be sure that a cast wouldn't break in rare cases. Oops. silly me, getLock can indeed return whatever. It would be really odd, but it's possible. Sorry for the noise ... No problem. I almost went with the cast approach on the basis that ReentrantLock was the only realistic option. It is good to have a check on these sorts of things. Thanks, Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: (tomcat) branch 10.1.x updated: Fix backport of BZ 66508 regression fix
On Fri, Jan 19, 2024 at 11:08 AM Mark Thomas wrote: > > > > On 19/01/2024 09:22, Rémy Maucherat wrote: > > On Thu, Jan 18, 2024 at 8:18 PM wrote: > >> > >> This is an automated email from the ASF dual-hosted git repository. > >> > >> markt pushed a commit to branch 10.1.x > >> in repository https://gitbox.apache.org/repos/asf/tomcat.git > >> > >> > >> The following commit(s) were added to refs/heads/10.1.x by this push: > >> new 67638c17e7 Fix backport of BZ 66508 regression fix > >> 67638c17e7 is described below > >> > >> commit 67638c17e7c19a0280ccafa340183fb179af92f5 > >> Author: Mark Thomas > >> AuthorDate: Thu Jan 18 18:52:30 2024 + > >> > >> Fix backport of BZ 66508 regression fix > > > > I don't understand the differences introduced. > > In SocketWrapperBase, the lock is not a ReentrantLock, but it still > > cannot be anything else: > > private final Lock lock = new ReentrantLock(); > > > > So couldn't the code be the same as in Tomcat 11 ? (with an extra > > cast, but no need to check anything) > > SocketWrapperBase.getLock() isn't final so it is possible for the > SocketWrapper implementation in a sub-class to override it and use a > different lock type. > > I think ReentrantLock is the only realistic option here but I was aiming > to be extra careful to avoid breakage. We do have some users that > implement custom connectors so I couldn't be sure that a cast wouldn't > break in rare cases. Oops. silly me, getLock can indeed return whatever. It would be really odd, but it's possible. Sorry for the noise ... Rémy > > Mark > > - > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: (tomcat) branch 10.1.x updated: Fix backport of BZ 66508 regression fix
On 19/01/2024 09:22, Rémy Maucherat wrote: On Thu, Jan 18, 2024 at 8:18 PM wrote: This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git The following commit(s) were added to refs/heads/10.1.x by this push: new 67638c17e7 Fix backport of BZ 66508 regression fix 67638c17e7 is described below commit 67638c17e7c19a0280ccafa340183fb179af92f5 Author: Mark Thomas AuthorDate: Thu Jan 18 18:52:30 2024 + Fix backport of BZ 66508 regression fix I don't understand the differences introduced. In SocketWrapperBase, the lock is not a ReentrantLock, but it still cannot be anything else: private final Lock lock = new ReentrantLock(); So couldn't the code be the same as in Tomcat 11 ? (with an extra cast, but no need to check anything) SocketWrapperBase.getLock() isn't final so it is possible for the SocketWrapper implementation in a sub-class to override it and use a different lock type. I think ReentrantLock is the only realistic option here but I was aiming to be extra careful to avoid breakage. We do have some users that implement custom connectors so I couldn't be sure that a cast wouldn't break in rare cases. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: (tomcat) branch 10.1.x updated: Fix backport of BZ 66508 regression fix
On Thu, Jan 18, 2024 at 8:18 PM wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch 10.1.x
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/10.1.x by this push:
> new 67638c17e7 Fix backport of BZ 66508 regression fix
> 67638c17e7 is described below
>
> commit 67638c17e7c19a0280ccafa340183fb179af92f5
> Author: Mark Thomas
> AuthorDate: Thu Jan 18 18:52:30 2024 +
>
> Fix backport of BZ 66508 regression fix
I don't understand the differences introduced.
In SocketWrapperBase, the lock is not a ReentrantLock, but it still
cannot be anything else:
private final Lock lock = new ReentrantLock();
So couldn't the code be the same as in Tomcat 11 ? (with an extra
cast, but no need to check anything)
Rémy
> ---
> .../server/WsRemoteEndpointImplServer.java | 61
> +++---
> 1 file changed, 31 insertions(+), 30 deletions(-)
>
> diff --git
> a/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
> b/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
> index d0cfef46ac..3be5d4f382 100644
> --- a/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
> +++ b/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
> @@ -107,42 +107,43 @@ public class WsRemoteEndpointImplServer extends
> WsRemoteEndpointImplBase {
> * Special handling is required only when all of the following are
> true:
> * - A close message is being sent
> * - This thread currently holds the socketWrapper lock (i.e. the
> thread is current processing a socket event)
> + *
> + * Special handling is only possible if the socketWrapper lock is a
> ReentrantLock (it will be by default)
> */
> -if (!(opCode == Constants.OPCODE_CLOSE &&
> socketWrapper.getLock().isHeldByCurrentThread())) {
> -// Skip special handling
> -return super.acquireMessagePartInProgressSemaphore(opCode,
> timeoutExpiry);
> -}
> -
> -int socketWrapperLockCount;
> if (socketWrapper.getLock() instanceof ReentrantLock) {
> -socketWrapperLockCount = ((ReentrantLock)
> socketWrapper.getLock()).getHoldCount();
> -} else {
> -socketWrapperLockCount = 1;
> -}
> -while (!messagePartInProgress.tryAcquire()) {
> -if (timeoutExpiry < System.currentTimeMillis()) {
> -return false;
> -}
> -try {
> -// Release control of the processor
> -socketWrapper.setCurrentProcessor(connection);
> -// Release the per socket lock(s)
> -for (int i = 0; i < socketWrapperLockCount; i++) {
> -socketWrapper.getLock().unlock();
> -}
> -// Provide opportunity for another thread to obtain the
> socketWrapper lock
> -Thread.yield();
> -} finally {
> -// Re-obtain the per socket lock(s)
> -for (int i = 0; i < socketWrapperLockCount; i++) {
> -socketWrapper.getLock().lock();
> +ReentrantLock reentrantLock = (ReentrantLock)
> socketWrapper.getLock();
> +if (opCode == Constants.OPCODE_CLOSE &&
> reentrantLock.isHeldByCurrentThread()) {
> +int socketWrapperLockCount = reentrantLock.getHoldCount();
> +
> +while (!messagePartInProgress.tryAcquire()) {
> +if (timeoutExpiry < System.currentTimeMillis()) {
> +return false;
> +}
> +try {
> +// Release control of the processor
> +socketWrapper.setCurrentProcessor(connection);
> +// Release the per socket lock(s)
> +for (int i = 0; i < socketWrapperLockCount; i++) {
> +socketWrapper.getLock().unlock();
> +}
> +// Provide opportunity for another thread to obtain
> the socketWrapper lock
> +Thread.yield();
> +} finally {
> +// Re-obtain the per socket lock(s)
> +for (int i = 0; i < socketWrapperLockCount; i++) {
> +socketWrapper.getLock().lock();
> +}
> +// Re-take control of the processor
> +socketWrapper.takeCurrentProcessor();
> +}
> }
> -// Re-take control of the processor
> -socketWrapper.takeCurrentProcessor();
> +
> +return true;
> }
> }
>
> -return true;
> +// Skip special handling
> +return
(tomcat) branch 10.1.x updated: Fix backport of BZ 66508 regression fix
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 67638c17e7 Fix backport of BZ 66508 regression fix
67638c17e7 is described below
commit 67638c17e7c19a0280ccafa340183fb179af92f5
Author: Mark Thomas
AuthorDate: Thu Jan 18 18:52:30 2024 +
Fix backport of BZ 66508 regression fix
---
.../server/WsRemoteEndpointImplServer.java | 61 +++---
1 file changed, 31 insertions(+), 30 deletions(-)
diff --git
a/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
b/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
index d0cfef46ac..3be5d4f382 100644
--- a/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
+++ b/java/org/apache/tomcat/websocket/server/WsRemoteEndpointImplServer.java
@@ -107,42 +107,43 @@ public class WsRemoteEndpointImplServer extends
WsRemoteEndpointImplBase {
* Special handling is required only when all of the following are
true:
* - A close message is being sent
* - This thread currently holds the socketWrapper lock (i.e. the
thread is current processing a socket event)
+ *
+ * Special handling is only possible if the socketWrapper lock is a
ReentrantLock (it will be by default)
*/
-if (!(opCode == Constants.OPCODE_CLOSE &&
socketWrapper.getLock().isHeldByCurrentThread())) {
-// Skip special handling
-return super.acquireMessagePartInProgressSemaphore(opCode,
timeoutExpiry);
-}
-
-int socketWrapperLockCount;
if (socketWrapper.getLock() instanceof ReentrantLock) {
-socketWrapperLockCount = ((ReentrantLock)
socketWrapper.getLock()).getHoldCount();
-} else {
-socketWrapperLockCount = 1;
-}
-while (!messagePartInProgress.tryAcquire()) {
-if (timeoutExpiry < System.currentTimeMillis()) {
-return false;
-}
-try {
-// Release control of the processor
-socketWrapper.setCurrentProcessor(connection);
-// Release the per socket lock(s)
-for (int i = 0; i < socketWrapperLockCount; i++) {
-socketWrapper.getLock().unlock();
-}
-// Provide opportunity for another thread to obtain the
socketWrapper lock
-Thread.yield();
-} finally {
-// Re-obtain the per socket lock(s)
-for (int i = 0; i < socketWrapperLockCount; i++) {
-socketWrapper.getLock().lock();
+ReentrantLock reentrantLock = (ReentrantLock)
socketWrapper.getLock();
+if (opCode == Constants.OPCODE_CLOSE &&
reentrantLock.isHeldByCurrentThread()) {
+int socketWrapperLockCount = reentrantLock.getHoldCount();
+
+while (!messagePartInProgress.tryAcquire()) {
+if (timeoutExpiry < System.currentTimeMillis()) {
+return false;
+}
+try {
+// Release control of the processor
+socketWrapper.setCurrentProcessor(connection);
+// Release the per socket lock(s)
+for (int i = 0; i < socketWrapperLockCount; i++) {
+socketWrapper.getLock().unlock();
+}
+// Provide opportunity for another thread to obtain
the socketWrapper lock
+Thread.yield();
+} finally {
+// Re-obtain the per socket lock(s)
+for (int i = 0; i < socketWrapperLockCount; i++) {
+socketWrapper.getLock().lock();
+}
+// Re-take control of the processor
+socketWrapper.takeCurrentProcessor();
+}
}
-// Re-take control of the processor
-socketWrapper.takeCurrentProcessor();
+
+return true;
}
}
-return true;
+// Skip special handling
+return super.acquireMessagePartInProgressSemaphore(opCode,
timeoutExpiry);
}
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
