On 28.07.2025 16:49, Rick McGuire wrote:
Once again, you didn't bother reading what I told you.
Why do you say that as it is not true? (I have always appreciated your
expertise and help.)
Under NO circumstances should you be altering the instance variable in
tmpInstance.
AFAIK I am *not* doing that!
Here the class definitions from Interpreter.hpp for InstanceBLock and InstanceAttacherDetacher, both
define a public field named instance:
class InstanceBlock
{
public:
InstanceBlock();
InstanceBlock(RexxOption *options);
InstanceBlock(PRXSYSEXIT exits, const char *env);
~InstanceBlock();
Activity *activity; // our current activity
/InterpreterInstance *instance; // potential interpreter instance/
};
class InstanceAttacherDetacher
{
public:
InstanceAttacherDetacher(InterpreterInstance *i);
~InstanceAttacherDetacher();
**/InterpreterInstance *instance; // interpreter instance/
};
Here the implementation of InstanceAttacherDetacher:
/**
* Make sure that attachThread() and detachThread() get carried out for the
* supplied InterpreterInstance.
*
* @param i InterpreterInstance
*/
InstanceAttacherDetacher::InstanceAttacherDetacher(InterpreterInstance *i)
{
// Get an instance.
instance = i;
instance->InterpreterInstance::attachThread();
}
/**
* Carry out detachThread() for the instance, if going out of scope.
*/
InstanceAttacherDetacher::~InstanceAttacherDetacher()
{
// detach from the instance
instance->InterpreterInstance::detachThread();
}
Here the code where tmpInstance's field gets referred to, but not changed:
// the resource lock needs to be released here because unloading
packages
// will require the kernel lock, which can never be requested while
holding
// the resource lock
try
{
// this may seem funny, but we need to create an instance
// to shut down so that the package manager can unload
// the libraries (it needs to pass a RexxThreadContext
// pointer out to package unloaders, if they are defined)
InstanceBlock tmpInstance;
*// make sure that attaching and detaching is done for this
particular block {*
InstanceAttacherDetacher instAttDet(*tmpInstance.instance*);
// run whatever uninits we can before we start releasing the
libraries
memoryObject.lastChanceUninit();
PackageManager::unload();
*}*
}
catch (ActivityException)
{
// we're shutting down, so ignore any failures while processing this
}
Where would a tmpInstance variable get changed?
Maybe I am not seeing the forest for the trees.
I'm not sure that's the cause of the problem, but that would be a very, very, bad thing to do and
it gains you nothing.
And of course, I would not want to do that at all!
---rony
On Mon, Jul 28, 2025 at 10:11 AM Rony G. Flatscher <rony.flatsc...@wu.ac.at>
wrote:
On 28.07.2025 15:02, Rony G. Flatscher wrote:
On 28.07.2025 13:51, Rick McGuire wrote:
You broke, you fix it.
:)
But having said that, I see one obvious problem. Updating the instance
field of an
InstanceBlock is a very, very, very bad idea. That was a new interpreter
instance that was
created for that block of code that will be automatically removed when it
goes out of scope.
Changing this means a) you are preventing that instance from getting
cleaned up when it's
supposed to, and b) when the destructor runs, you are cleaning up the
instance that has
already been detached. The dispatching internals are going to be seriously
messed up.
Thanks. Enclosed the code in a proper block to make sure that the instance
is available:
// the resource lock needs to be released here because unloading
packages
// will require the kernel lock, which can never be requested
while holding
// the resource lock
try
{
// this may seem funny, but we need to create an instance
// to shut down so that the package manager can unload
// the libraries (it needs to pass a RexxThreadContext
// pointer out to package unloaders, if they are defined)
InstanceBlock tmpInstance;
*// make sure that attaching and detaching is done for this
particular block {*
InstanceAttacherDetacher instAttDet(tmpInstance.instance);
// run whatever uninits we can before we start releasing
the libraries
memoryObject.lastChanceUninit();
PackageManager::unload();
*}*
}
catch (ActivityException)
{
// we're shutting down, so ignore any failures while
processing this
}
Ran the testsuite on Windows and it did not crash anymore, hence planning
to commit this
version to see how it fares on the Jenkins farm.
Well, that was just by coincidence that it did not crash.
My understanding from C++ would be that the block in which InstanceBlock
tmpInstance is
defined is the outer block relative to the new block in which
InstanceAttacherDetacher
instAttDet(tmpInstance.instance) gets defined and accesses tmpInstance's
instance. If the
inner block is left then the InstanceAttacherDetacher destructor gets
invoked before arriving
in the outer block such that the InstanceBlock destructor gets run
afterwards when the outer
block is left.
If that is not a safe solution, what would be one?
---rony
_______________________________________________
Oorexx-devel mailing list
Oorexx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/oorexx-devel
