[Geany-Devel] On document pointer recycling

Lex Trotman elextr at xxxxx
Sat Oct 26 13:22:37 UTC 2013


On 26 October 2013 23:17, Nick Treleaven <nick.treleaven at btinternet.com>wrote:

> On 26/10/2013 00:22, Lex Trotman wrote:
>
>> On 26 October 2013 00:01, Nick Treleaven <nick.treleaven at btinternet.com**
>> >wrote:
>>
>>  On 24/10/2013 10:24, Lex Trotman wrote:
>>>
>>>  Okay, but you still agree that doc->is_valid should be removed
>>>> eventually?
>>>>
>>>>> That's a step forward:)
>>>>>>
>>>>>
>>>>>
>>>> Of course I agree.  So its not a terribly big step:)
>>>>
>>>>
>>> I only skimmed the discussion, but how can we remove that?
>>>
>>>
>> Given your experience it would be good if you could read the discussion
>> and
>> give your perspective on what the previous posters have discussed
>> regarding
>> handling doc pointers properly.
>>
>
> I read the discussion.
>
> In summary, I think there are 3 issues:
> * forgetting to check is_valid when iterating documents
> * freeing memory
> * code that uses is_valid to check if a document still exists, which is
> bad practice
>

Not just bad practice, but wrong, it does not guarantee its the same
document.  Its unlikely, but possible, if the holder of the pointer does
not correctly handle document close.


>
> I think freeing memory is a non-issue, I think you (Lex) agree with me on
> that. It's not a memory leak, it may be reused later.
>

Yes agree, the memory is not technically "lost" and the amount does not
grow over time, although its re-use depends on the users usage pattern,
which I think was what Thomas was concerned about.  But if you had enough
memory to open that many documents with all the scintilla overhead, then
keeping the GeanyDocument struct isn't material.


>
> The first point needs to be handled before the last one (assuming each are
> worth fixing). I think we can:
>

If Geany or any plugin keeps a pointer (or an index) to a document they
have to properly handle document close, even with the current
implementation, to ensure that their pointer (or index) points to the same
document, and if they handle that properly they won't have an invalid
pointer or index.  That requirement is independent of how the document list
is implemented, and it should remove all need to use is_valid outside
iteration.

Then the issue of forgetting during iteration can be fixed.


>
> * remove GeanyDocument::index and fix any code that uses it
> * review all code indexing documents_array and fix any code retaining an
> index (I expect that is rarer than code using GeanyDocument::index).
> * change documents_array to only contain valid documents, i.e. on close we
> swap the last valid document pointer with the pointer being removed, and
> decrement the length by 1.
> * use a free list for invalid documents
>
> The last point is important to ensure no memory corruption bugs are
> introduced. I think it is harder to find the code that retains document
> pointers (and there is some), so I recommend we don't free document memory.
> At least, we would need a better reason to do so IMO than just memory
> usage, which is insignificant.
>

Yes, the memory is not an issue, but the real problem (not handling close)
needs to be fixed wherever it occurs, and changing the implementation won't
help.  In fact, as I said in the PS that started this thread, the current
implementation at least ensures that the pointer/index will always
reference something that is a GeanyDocument object, if the document structs
were de-allocated in the normal way the pointer could point to anything.

Although the chances of the wrong document being referenced because of a
retained pointer are slim, any fault that can happen, will happen (and at
the worst moment according to Murphy).


>
> Now, once we've done that it's no longer needed to check is_valid when
> iterating documents. Possibly we could remove is_valid, but I think that is
> less important to do. Note that if we do all the steps above, we don't need
> to break the API, only the ABI. I can't guarantee that bugs won't be
> introduced due to these changes, but it seems fairly reliable.


An API break doesn't matter really if we are going to break the ABI, that
requires all plugins to be recompiled anyway.  So that says we should only
do this when something forces an ABI break anyway.

Cheers
Lex



>
> ______________________________**_________________
> Devel mailing list
> Devel at lists.geany.org
> https://lists.geany.org/cgi-**bin/mailman/listinfo/devel<https://lists.geany.org/cgi-bin/mailman/listinfo/devel>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.geany.org/pipermail/devel/attachments/20131027/f78a9fc5/attachment.html>


More information about the Devel mailing list