You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/12/16 21:07:22 UTC
[GitHub] [activemq-nms-openwire] troutkilroy opened a new pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
troutkilroy opened a new pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12
While the .NET Apache library is compiled to use DedicatedTaskRunner, we prefer to compile from source and enable PooledTaskRunner (as set in TaskRunnerFactory.cs). However PooledTaskRunner.cs has an obvious issue with shutdown() as it will block on an infinite sleep if the task is busy (i.e., iterating == true). Not sure what was the intent of the sleep() calls, but the Java version of this code we believe is correct with event notification to control shutdown of the task. This change basically brings the C# implementation in line with that using Monitor (similar to what was done with the C# implementation of DedicatedTaskRunner).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] michaelandrepearce commented on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-748453026
@Havret if youre doing a respin, do you think its worth adding this, or it should wait till after?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] michaelandrepearce commented on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-754991213
@troutkilroy i was about to look to merge this, but noticed there is no test case, can you add a test case for this please.
a) to demonstrate the issue
b) prove that this fix does fix the issue
c) ensure the issue is not regressed by mistake
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] troutkilroy commented on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
troutkilroy commented on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-763987745
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] troutkilroy commented on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
troutkilroy commented on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-763987745
Sorry for late response. Yes I created a test case for this, and it will demonstrate failure with the current code and success with fix,. Also I have a correction to the code I submitted. We can cancel this pull, and I'll submit a new one.
One question though. For the test case I added PooledTaskRunnerTest.cs file to tests/threads. However unlike the other taskrunner implementations the PooledTaskRunner class is not declared public, so I either make it public or I'll have to add an assembly config to the main project with an InternalsVisibleTo attribute pointing at the test assembly. Making it public would be easier. Which do you prefer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] troutkilroy commented on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
troutkilroy commented on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-764067484
Closing I'll resubmit new pull with test cases. Also if I could get clarification on making PooledTaskRunner public or not as noted above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] troutkilroy closed pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
troutkilroy closed pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] troutkilroy closed pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
troutkilroy closed pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] troutkilroy edited a comment on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
troutkilroy edited a comment on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-763987745
Sorry for late response. Yes I created a test case for this, and it will demonstrate failure with the current code and success with fix, Also I have a correction to the code I submitted. We can cancel this pull, and I'll submit a new one.
One question though. For the test case I added PooledTaskRunnerTest.cs file to tests/threads. However unlike the other taskrunner implementations the PooledTaskRunner class is not declared public, so I either make it public or I'll have to add an assembly config to the main project with an InternalsVisibleTo attribute pointing at the test assembly. Making it public would be easier. Which do you prefer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] michaelandrepearce commented on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-764192968
I have no issue if you make it public
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] troutkilroy edited a comment on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
troutkilroy edited a comment on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-763987745
Sorry for late response. Yes I created a test case for this, and it will demonstrate failure with the current code and success with fix, Also I have a correction to the code I submitted. We can cancel this pull, and I'll submit a new one.
One question though. For the test case I added PooledTaskRunnerTest.cs file to tests/threads. However unlike the other taskrunner implementations the PooledTaskRunner class is not declared public, so I either make it public or I'll have to add an assembly config to the main project with an InternalsVisibleTo attribute pointing at the test assembly. Making it public would be easier. Which do you prefer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] michaelandrepearce commented on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-764192968
I have no issue if you make it public
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] michaelpearce-gain commented on a change in pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#discussion_r560837334
##########
File path: src/Threads/PooledTaskRunner.cs
##########
@@ -106,34 +106,34 @@ public void Shutdown(TimeSpan timeout)
{
if(iterating)
{
- System.Threading.Thread.Sleep(timeout);
+ Monitor.Wait(runable, timeout);
}
}
}
}
- public void ShutdownWithAbort(TimeSpan timeout)
+ public void ShutdownWithAbort(TimeSpan timeout)
+ {
+ lock(runable)
+ {
+ _shutdown = true;
+
+ if (runningThread != Thread.CurrentThread)
{
- lock(runable)
- {
- _shutdown = true;
-
- if (runningThread != Thread.CurrentThread)
- {
- if(iterating)
- {
- Thread.Sleep(timeout);
- }
-
- if(iterating)
- {
- runningThread.Abort();
+ if(iterating)
+ {
+ Monitor.Wait(runable, timeout);
Review comment:
formatting seems to a bit over the place
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] michaelandrepearce edited a comment on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
michaelandrepearce edited a comment on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-754991213
@troutkilroy i was about to look to merge this, but noticed there is no test case, can you add a test case for this please.
a) to demonstrate the issue
b) prove that this fix does fix the issue
c) ensure the issue is not regressed by
as well can you raise a JIRA item, https://issues.apache.org/jira/projects/AMQNET/issues/ for the issue, and update the commit message to contain prefix with the Jira item.
Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-nms-openwire] michaelpearce-gain commented on pull request #12: Implemented necessary signaling for Shutdown() and ShutDownWithAbort()
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #12:
URL: https://github.com/apache/activemq-nms-openwire/pull/12#issuecomment-784076511
@troutkilroy are you re-submitting? Or shall i close the issue out?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org