Solved: Re: API bug - 2018.1 causes crash using UpgradeOpen on Dependent LayerTableRecor

Anonymous · ‎08-02-2017

Hello,

I seem to have found an API bug in AutoCAD 2018.1 update. The code below works fine prior to update, but crashes after installing update. I've narrowed down the problem as such...

Bug explained in one sentence:

If a Dependent LayerTableRecord is initially opened ForRead, and UpgradeOpen is used before setting IsLocked or IsFrozen to True, AutoCAD will crash once the transaction is committed.

This code crashes in 2018.1

 using (Transaction tr = WorkingDatabase.TransactionManager.StartTransaction())
            {

                LayerTable lt = tr.GetObject(WorkingDatabase.LayerTableId, OpenMode.ForWrite);


                foreach (ObjectId LayId in lt)
                {

                    LayerTableRecord lay = tr.GetObject(LayId, OpenMode.ForRead);

                    if (lay.IsDependent)
                    {
                        lay.UpgradeOpen();

                        lay.IsLocked = true;
                        lay.IsFrozen = true;

                    }

                }

                tr.Commit();

            }

Where affected, I have modified my application code to ALWAYS open LayerTableRecords ForWrite. See modified code below...

This code does NOT crash in 2018.1

 using (Transaction tr = WorkingDatabase.TransactionManager.StartTransaction())
            {

                LayerTable lt = tr.GetObject(WorkingDatabase.LayerTableId, OpenMode.ForWrite);


                foreach (ObjectId LayId in lt)
                {

                    LayerTableRecord lay = tr.GetObject(LayId, OpenMode.ForWrite);

                    if (lay.IsDependent)
                    {
                        lay.IsLocked = true;
                        lay.IsFrozen = true;

                    }

                }

                tr.Commit();

            }

Does anyone see an issue with my code that could be the problem? If not, would be nice to have a case opened to track when the issue is fixed.

Thanks,

Jon Albert

ActivistInvestor · ‎08-02-2017

LayerTable lt = tr.GetObject(WorkingDatabase.LayerTableId, OpenMode.ForWrite);

LayerTableRecord lay = tr.GetObject(LayId, OpenMode.ForRead);

Neither of your snippets should compile without a typecast on the result of calls to GetObject().

LayerTableRecord lay = (LayerTableRecord) tr.GetObject(LayId, OpenMode.ForRead);

You might want to try both of them using an OpenCloseTransaction(), and calling DowngradeOpen() immediately after setting the two layer properties.

I always test code using both a Transaction and an OpenCloseTransaction, because there are subtle differences in how they behave and I need the code to work with either type. For example, when using Transaction, DownGradeOpen() does nothing, and the object remains open for write until the transaction is committed. With OpenCloseTransaction, DowngradeOpen() closes the object, commits the changes made to it, and reopens it for read immediately.

Anonymous · ‎08-02-2017

Thanks @ActivistInvestor,

I tried your suggestions with interesting results. First though, I have question regarding TypeCast comment. My code is actually in VB.NET, and I used Code Converter to create the C# for post. Below is my VB.NET code, I'm guessing the TypeCast doesn't apply with VB.NET, because I've been doing this way for years, and all online code samples show same.

 Using tr As Transaction = WorkingDatabase.TransactionManager.StartTransaction()

            Dim lt As LayerTable = tr.GetObject(WorkingDatabase.LayerTableId, OpenMode.ForWrite)

            For Each LayId As ObjectId In lt

                Dim lay As LayerTableRecord = tr.GetObject(LayId, OpenMode.ForRead)

                If lay.IsDependent Then

                    lay.UpgradeOpen()

                    lay.IsLocked = True
                    lay.IsFrozen = True

                End If

            Next

            tr.Commit()

        End Using

I tried your suggestions regarding Transaction vs. OpenCloseTransaction, and use of DowngradeOpen. The following are my findings...

Scenario 1:

Using my original code (with Transaction and UpgradeOpen), then simply calling DowngradeOpen after setting layer properties does prevent AutoCAD from crashing.

Scenario 2:

Using OpenCloseTransaction instead of Transaction also prevents AutoCAD from crashing.

Scenario 3:

Same as Scenario 2, plus calling DowngradeOpen after setting layer properties also prevents AutoCAD from crashing.

Based on these results, perhaps you got this backwards @ActivistInvestor? "...when using Transaction, DownGradeOpen() does nothing, and the object remains open for write until the transaction is committed. With OpenCloseTransaction, DowngradeOpen() closes the object, commits the changes made to it, and reopens it for read immediately."

For now I prefer to use Scenario 1 as workaround to avoid unnecessarily opening LayerTableRecord ForWrite.

However (to the ADN team), unless I'm missing something, this does seem like a bug in 2018.1 API. According to documentation, DowngradeOpen should not have to be called, and only in this specific case for 2018.1 has it suddenly become necessary.

Jon Albert

artc2 · ‎08-02-2017

UpgradeOpen() is a wrapper for the C++ AcDbObject::upgradeOpen() which is for the open/close mechanism that runs along side the regular transaction mechanism. Since you are working with a transaction and it's not an open/close transaction, instead of using UpgradeOpen(), you should do another tr.GetObject(), but do it for write instead of for read. That way the open for write will be part of the transaction. And, you don't need to assign the return value from that tr.GetObject since you already have your lt variable set to the LayerTable object and that isn't changing.

Mixing open/close with regular transactions on the same object at the same time is fragile and can result in issues.

ActivistInvestor · ‎08-02-2017

@Anonymous wrote:
Thanks @ActivistInvestor,

I tried your suggestions with interesting results. First though, I have question regarding TypeCast comment. My code is actually in VB.NET, and I used Code Converter to create the C# for post. Below is my VB.NET code, I'm guessing the TypeCast doesn't apply with VB.NET, because I've been doing this way for years, and all online code samples show same.

Scenario 1:
Using my original code (with Transaction and UpgradeOpen), then simply calling DowngradeOpen after setting layer properties does prevent AutoCAD from crashing.
Scenario 2:
Using OpenCloseTransaction instead of Transaction also prevents AutoCAD from crashing.
Scenario 3:
Same as Scenario 2, plus calling DowngradeOpen after setting layer properties also prevents AutoCAD from crashing.

Based on these results, perhaps you got this backwards @ActivistInvestor? "...when using Transaction, DownGradeOpen() does nothing, and the object remains open for write until the transaction is committed. With OpenCloseTransaction, DowngradeOpen() closes the object, commits the changes made to it, and reopens it for read immediately."

For now I prefer to use Scenario 1 as workaround to avoid unnecessarily opening LayerTableRecord ForWrite.

However (to the ADN team), unless I'm missing something, this does seem like a bug in 2018.1 API. According to documentation, DowngradeOpen should not have to be called, and only in this specific case for 2018.1 has it suddenly become necessary.

Jon Albert

In VB.NET the typecast is not required, because it is done implicitly by the compiler.

AFAIK, that's how regular transactions have always worked. The object remains open for write until the transaction is committed.

The problem could in-fact be caused by the fact that the object is open for write when the transaction is committed.

The fact that you see different behavior between product updates is a reason for concern.

Anonymous · ‎08-02-2017

Thanks for the confirmation on TypeCast for VB.NET.

Hopefully ADN will pick this up to investigate and provide confirmation of issue/fix.

Jon

Anonymous · ‎08-02-2017

Interesting @artc2,

Your suggestion also fixes the problem. Simply replacing lay.UpgradeOpen() with tr.GetObject(LayId, OpenMode.ForWrite) also prevents the crash. Let's call this Scenario 4, my preferred "workaround", and perhaps a personal best practice going forward! To be honest, I'd rather not know this and be happy using UpgradeOpen, ignorance is bliss

Perhaps you are correct in saying "Mixing open/close with regular transactions on the same object at the same time is fragile and can result in issues.". Nevertheless, we should be able to rely on UpgradeOpen inside a regular Transaction as sample code on support sites show this as standard method. https://knowledge.autodesk.com/search-result/caas/CloudHelp/cloudhelp/2015/ENU/AutoCAD-NET/files/GUI...

Thanks!

Jon

artc2 · ‎08-02-2017

UpgradeOpen() and the C++ upgradeOpen() that it wraps are both intended to be used on an object that is already open for read via the normal open/close mechanism. In your case you were calling it on an object that was only open in a transaction NOT in a normal open. That leads to strange flag settings in the object itself which can lead to problems. In this case there is new code in the update that isn't expecting this and it happens to cause a crash.

I haven't tried it, but I would expect that if you used an openclose transaction instead of a regular transaction, then the UpgradeOpen ought to work.

Anonymous · ‎08-02-2017

Indeed using OpenCloseTransaction with UpgradeOpen does work as I mention in Scenario 2 above.

My qualm is that you refer to OpenCloseTransaction as the "normal open/close mechanism". Every transaction I've ever coded uses Transaction with UpgradeOpen, as do all official code samples I've ever seen.

Just to clarify, you don't recommend following the official code samples in link below? Ideally the official code samples would use StartOpenCloseTransaction instead of StartTransaction when UpgradeOpen is required (or replace UpgradeOpen with tr.GetObject(Id, OpenMode.ForWrite) ) ?

https://knowledge.autodesk.com/search-result/caas/CloudHelp/cloudhelp/2015/ENU/AutoCAD-NET/files/GUI...

ActivistInvestor · ‎08-02-2017

@Anonymous wrote:
Indeed using OpenCloseTransaction with UpgradeOpen does work as I mention in Scenario 2 above.

My qualm is that you refer to OpenCloseTransaction as the "normal open/close mechanism". Every transaction I've ever coded uses Transaction with UpgradeOpen, as do all official code samples I've ever seen.

Just to clarify, you don't recommend following the official code samples in link below? Ideally the official code samples would use StartOpenCloseTransaction instead of StartTransaction when UpgradeOpen is required (or replace UpgradeOpen with tr.GetObject(Id, OpenMode.ForWrite) ) ?

https://knowledge.autodesk.com/search-result/caas/CloudHelp/cloudhelp/2015/ENU/AutoCAD-NET/files/GUID-CC5CC229-B122-4897-A8DA-5C5ADADB0F38-htm.html

I think Art would agree that you probably shouldn't follow those samples, especially if they are broken by the 2018.1 update. The fact is that there is a lot of existing code out there that uses UpgradeOpen() on transaction-resident objects (including my own), that is quite-likely going to break on 2018.1. I've also never read (in the native ObjectARX docs or anywhere else), that UpgradeOpen() was not intended for use with transaction-resident objects, but if that's what Art says, then that's what it is.

Not only can you find code samples, but a quick analysis with Reflector reveals that there is quite a bit of Autodesk-produced managed code that comes in the box, which also uses UpgradeOpen() on transaction-resident objects.

By the 'normal open/close mechanism', he refers to manually opening and closing objects (which in the managed API is done via ObjectId.Open(), and DBObject.Close() or DBObject.Cancel()). The OpenCloseTransaction uses the same underlying native APIs called by those managed APIs to open and close objects, so it is essentially just another way of doing the same thing. The OpenCloseTransaction exists because there are many cases where objects can't be opened in normal Transactions (mainly from within event handlers, overrules, etc.), and using ObjectId.Open() and DBObject.Close() is somewhat more complicated and error-prone, so the OpenCloseTransaction was created to provide a more convenient and less-complicated way to use the 'normal open/close mechanism'.

You can always tell if an object was opened via a transaction or via the normal open/close mechanism by looking at its IsTransactionResident property.

Anonymous · ‎08-02-2017

Ok thanks for further clarification. Suppose I will re-visit some code to future/bullet proof things. From what I've read today, OpenCloseTransaction cannot be nested, so I will take that into consideration as well. http://spiderinnet1.typepad.com/blog/2012/08/autocad-net-openclosetransaction-can-it-be-nested.html

This has been enlightening!

Jon

ActivistInvestor · ‎08-02-2017

@Anonymous wrote:
Ok thanks for further clarification. Suppose I will re-visit some code to future/bullet proof things. From what I've read today, OpenCloseTransaction cannot be nested, so I will take that into consideration as well. http://spiderinnet1.typepad.com/blog/2012/08/autocad-net-openclosetransaction-can-it-be-nested.html

This has been enlightening!

Jon

I don't see the comments at the link you posted as being too meaningful. The OpenCloseTransaction exists because there are situations where a 'regular transaction' cannot be used, or cannot be used without screwing-up UNDO/REDO. Yea, it;s faster than a regular transaction, and how much faster depends on how many objects are being operated on, available memory, and plenty of other variables, but marginally-better performance is not the reason why it exists.

I also don't think nesting transactions is a common practice, or at least, not if one's code is designed to share a single transaction. Most of my helper APIs take a Transaction as an argument, which can be either an OpenCloseTransaction or a regular Transaction. Starting and ending regular nested Transactions in helper APIs is IMO, not a good practice, because that makes those APIs incapable of using the OpenCloseTransaction when it is necessary (e.g., from code that's called from an event handler, overrule, etc.) and therefore imposes restrictions on the contexts in which the API can be used.

ActivistInvestor · ‎08-02-2017

@Anonymous wrote:
Ok thanks for further clarification. Suppose I will re-visit some code to future/bullet proof things. From what I've read today, OpenCloseTransaction cannot be nested, so I will take that into consideration as well.

This has been enlightening!

Jon

I don't see the comments at the link you posted as being too meaningful. The OpenCloseTransaction exists because there are situations where a 'regular transaction' cannot be used, or cannot be used without screwing-up UNDO/REDO. Yea, it;s faster than a regular transaction, and how much faster depends on how many objects are being operated on, available memory, and plenty of other variables, but marginally-better performance is not the reason why it exists.

I also don't think nesting transactions is a common practice, or at least, not if one's code is designed to share a single transaction. Most of my helper APIs take a Transaction as an argument, which can be either an OpenCloseTransaction or a regular Transaction. Starting and ending regular nested Transactions in helper APIs is IMO, not a good practice, because that makes those APIs incapable of using the OpenCloseTransaction when it is necessary (e.g., from code that's called from an event handler, overrule, etc.) and therefore imposes restrictions on the contexts in which the API can be used.

Anonymous · ‎08-02-2017

I see, nesting transactions does seem less than ideal, and with closer look at my own code it appears I have avoided it.

As for OpenCloseTransaction, guess I've never encountered situation where it was needed for me. Not to say I wont someday, just that I haven't coded anything of that nature. For now it's safe to say I can leave all my code with "regular transaction".

So I think it all makes sense to me now, and all I really need to concern myself with is using tr.GetObject(Id, OpenMode.ForWrite) instead of UpgradeOpen. This should protect me against unexpected results.

ActivistInvestor · ‎08-02-2017

@Anonymous wrote:
I see, nesting transactions does seem less than ideal, and with closer look at my own code it appears I have avoided it.

As for OpenCloseTransaction, guess I've never encountered situation where it was needed for me. Not to say I wont someday, just that I haven't coded anything of that nature. For now it's safe to say I can leave all my code with "regular transaction".

So I think it all makes sense to me now, and all I really need to concern myself with is using tr.GetObject(Id, OpenMode.ForWrite) instead of UpgradeOpen. This should protect me against unexpected results.

Because this affects a great deal of existing managed code (including Autodesk's own), I've already started on addressing it in my own code base.

This workaround is intended as an interim solution that requires minimal revisions to existing code, and is useful in cases where code may have to deal with objects that were opened via any type of transaction:

   public static class UpgradeOpenExtensions
   {
      /// <summary>
      /// Intended as an interim workaround for an issue in AutoCAD 2018.1, 
      /// where calling DBObject.UpgradeOpen() on a transaction-resident 
      /// DBObject crashes AutoCAD.
      /// 
      /// The workaround is designed to minimize the required changes to 
      /// existing code. It only requires that calls to UpgradeOpen() be 
      /// replaced with calls to the following extension method.
      /// 
      /// This method will avoid calls to DBObject.UpgradeOpen() on all
      /// database- and transaction-resident DBObjects it is invoked on,
      /// by calling the GetObject() method of the TransactionManager for
      /// the Database that owns the given DBObject.
      /// </summary>
      /// <param name="dbObject">The DBObject whose OpenMode is to be upgraded</param>
      
      public static void SafeUpgradeOpen(this DBObject dbObject)
      {
         if(dbObject == null)
            throw new ArgumentNullException("dbObject");
         if(dbObject.Database != null && !dbObject.IsWriteEnabled)
         {
            if(dbObject.IsTransactionResident)
            {
               dbObject.Database.TransactionManager.GetObject(dbObject.ObjectId,
                  OpenMode.ForWrite, true, false);
            }
            else
            {
               dbObject.UpgradeOpen();
            }
         }
      }

   }

Anonymous · ‎08-03-2017

My final thought on this... I would mark one of the "workarounds" as a solution, but still view the initial problem as an API bug unless there is an official statement that says to avoid using UpgradeOpen within 'regular transaction'.

Jon

artc2 · ‎08-03-2017

The crash you are seeing is due to a combination of the improper use of UpgradeOpen which sets internal flags incorrectly, and new code in the 2018.1 update which just happens to be sensitive to the flags being improperly set. We are looking at ways to change the code so that it is not sensitive to the improper flag settings.

ActivistInvestor · ‎08-03-2017

@Anonymous wrote:

As for OpenCloseTransaction, guess I've never encountered situation where it was needed for me. Not to say I wont someday, just that I haven't coded anything of that nature. For now it's safe to say I can leave all my code with "regular transaction".

Some folks use a regular Transaction not realizing they shouldn't do so, because they're unaware of the problems it can cause. For example, you can find lots of code posted here that uses regular Transactions within event handlers that fire while a command is in progress. The programmer made the mistake because they weren't aware of the problems that can that can cause, and didn't bother to test their code, especially with regards to UNDO/REDO (in some cases, modifying objects opened via a regular Transaction while a command is in progress will disable the user's ability to REDO an UNDO).

As a general rule, I always use an OpenCloseTransaction in code that can execute while a command is in progress, such as that called from an event handler, or an Overrule.

norman.yuan · ‎08-07-2017

Reading this post made me very nervous, because I have been so much used to open DBObject for read and the call UpgradeOpen() when needed. If this bug invalids using of UpgradeOpen, the impact would be huge, like the other replier even jumps on writing an extension to replace call to UpgradeOpen().

Acad2018.1 was releaded on July 27th (over 10 days by now), it would have been much more uproar by now if the bug as I thought. So, even we do not have plan to go AutoCAD2018 any time soon, I decided to try it out myself this weekend. After installed Acad2018.1, I ran the same code as posted, against a drawing with Xreferences attached (thus, there are layers that is dependent) Here are the results:

1. UpgradeOpen() works as usual to all other DBObjects than LayerTableRecord, at least with a few DBObjects/Entities I tried;

2. UpgradeOpen() works OK with regular LayerTableRecord, but not dependent layer (IsDependent=true, i.e. layer brought in from Xreference);

3. It is actually not the UpgradeOpen() call stops AutoCAD. In my debugging, the execution of calling UpgradeOpen() and then changing property (such as IsLocked) on dependent layertablerecord goes OK. It is when Transaction.Commit() is called, AutoCAD becomes hang (forever when not debugging; and raise "ContextSwitchDeadLock" exception when debugging)

4. Yes, if the LayerTableRecord is opened for Write (thus, no need to call UpgradeOpen()), then AutoCAD works the code through.

So, yes, it looks like a bug introduced by Acad2018.1, but it only is related/applied to dependent layers. I was relieved, at least for now, because I do not have code to Open/UpgradeOpen dependent layers so far, fortunately. No worry to the good, well-known approach of "open-for-read-and-upgradeopen-when-necessary".

Hopefully, ADN team member could confirm/verify this issue.

Norman Yuan

Drive CAD With Code

artc2 · ‎08-07-2017

1. UpgradeOpen() works as usual to all other DBObjects than LayerTableRecord, at least with a few DBObjects/Entities I tried;

UpgradeOpen is fine to use, but it should only be used on objects that have been opened via an OpenCloseTransaction or DBObject.Open.

I should also mention that DBObject.Open and OpenCloseTransaction are wrappers for the regular C++ open/close mechanism and regular transactions are a different open/close mechanism that runs alongside of the regular open/close mechanism. So, it is possible to have an object that is open by both mechanisms at the same time. If an object is open by both mechanisms at the same time, then it is ok to use UpgradeOpen on that object.

2. UpgradeOpen() works OK with regular LayerTableRecord, but not dependent layer (IsDependent=true, i.e. layer brought in from Xreference);

If the non-dependent LayerTableRecord is only open in a regular transaction, then UpgradeOpen should still no be used on it. It will work, but it is still incorrectly setting flags internally and should be avoided.

3. It is actually not the UpgradeOpen() call stops AutoCAD. In my debugging, the execution of calling UpgradeOpen() and then changing property (such as IsLocked) on dependent layertablerecord goes OK. It is when Transaction.Commit() is called, AutoCAD becomes hang (forever when not debugging; and raise "ContextSwitchDeadLock" exception when debugging)

That's correct. It's when the LayerTableRecord is truly closing that the problem occurs.

4. Yes, if the LayerTableRecord is opened for Write (thus, no need to call UpgradeOpen()), then AutoCAD works the code through.

In that case there are no incorrectly set flags internally, so no problem.

API bug - 2018.1 causes crash using UpgradeOpen on Dependent LayerTableRecord

API bug - 2018.1 causes crash using UpgradeOpen on Dependent LayerTableRecord