Thread safety check of MemorySegment

Wed Jul 22 11:33:24 UTC 2020

>
> On 22/07/2020 12:07, Johannes Lichtenberger wrote:
>
> I just had a quick look at the code, but I think I'm wrong. If
> transactions are not closed by it's transaction instance handle directly
> they are closed by the resource manager (because it keeps track of the
> started transactions) and the resource manager often times is opened on the
> main event loop (that might not be needed always, but still). I think in
> the long-run it really complicates stuff and we have to be super careful.
> It might even be impossible to know in advance on which thread a coroutine
> will be executed.
>
> So... you have a thread which creates (and accesses the segment).
>
> But then you have another thread which can close the segment.
>
> How does your logic make sure that the second thread cannot close the
> segment while the first is still operating on it? Is there a point in the
> code where the original thread hand over control to the second thread (the
> transaction one) ?
>
Basically it's not checked, just documented in the JavaDoc, that only one
thread at a time should handle the transaction. I think it's at least bad
style and I should refactor this.

So, for instance a time travel query might internally open a bunch of
transactions, as a transaction is always bound to one revision, thus it can
only access this version of the persistent, durable tree. The resource
manager however is sometimes opened in the other thread. The serializer,
which materializes the nodes / items as of now doesn't close the
transactions itself, thus the resource manager from the other thread has to
close the transactions. That might be not necessary.

I think in my case through careful thinking and refactoring I can get rid
of this, such that the database + resource manager + transactions are
always opened and closed in the same thread. I think code reviews would be
the best (also regarding clean code principles), but currently I'm lacking
an Open Source community as I didn't release a major version, that is
1.0.0. Well well, we'll see :-)

kind regards
Johannes

Am Mi., 22. Juli 2020 um 13:10 Uhr schrieb Maurizio Cimadamore <
maurizio.cimadamore at oracle.com>:

>
> On 22/07/2020 12:07, Johannes Lichtenberger wrote:
>
> I just had a quick look at the code, but I think I'm wrong. If
> transactions are not closed by it's transaction instance handle directly
> they are closed by the resource manager (because it keeps track of the
> started transactions) and the resource manager often times is opened on the
> main event loop (that might not be needed always, but still). I think in
> the long-run it really complicates stuff and we have to be super careful.
> It might even be impossible to know in advance on which thread a coroutine
> will be executed.
>
> So... you have a thread which creates (and accesses the segment).
>
> But then you have another thread which can close the segment.
>
> How does your logic make sure that the second thread cannot close the
> segment while the first is still operating on it? Is there a point in the
> code where the original thread hand over control to the second thread (the
> transaction one) ?
>
> Maurizio
>
>
> kind regards
> Johannes
>
> Am Mi., 22. Juli 2020 um 12:58 Uhr schrieb Johannes Lichtenberger <
> lichtenberger.johannes at gmail.com>:
>
>> Transfer of ownership must be initiated by the thread that owns the
>>> segment. If that works for you, then, yes, it can be used as a solution.
>>>
>>
>> I think in my case the code is not so much. I'm using Vert.x with
>> Kotlin/Coroutines for a REST-API and that currently is where the problem
>> stems from. I think I can even in most cases open the transaction in the
>> "worker" thread instead of the main loop. Thus, I guess it's doable without
>> too much work. In my case I might also be lucky, that I basically as of now
>> have no external projects, which embed SirixDB. In any case the read-write
>> transaction started in SirixDB also should be only used by one thread at a
>> time, but I'm simply not checking if it runs on another thread when closing.
>>
>> Tracking down the issue in my case was a bit annoying, because the
>> exception was caused in the close-method of the transaction and then the
>> transaction didn't close properly, but the exception wasn't thrown --
>> there's clearly an issue somehow with the exception handling in my REST-API
>> as it simply should be returned from the server :-/ Just during a resource
>> deletion, an exception popped up, that open resource managers (which start
>> transactions) were found.
>>
>> kind regards
>> Johannes
>>
>> Am Mi., 22. Juli 2020 um 12:43 Uhr schrieb Maurizio Cimadamore <
>> maurizio.cimadamore at oracle.com>:
>>
>>>
>>> On 22/07/2020 11:26, Johannes Lichtenberger wrote:
>>>
>>> I'm also using a mapped memory segment. But I think at least we have the
>>> possibility to transfer ownership in Java 15, right? Thus, in my
>>> AutoClosable in the close() method I can simply transfer ownership to the
>>> current thread if the segment is still alive :-)
>>>
>>> Transfer of ownership must be initiated by the thread that owns the
>>> segment. If that works for you, then, yes, it can be used as a solution.
>>>
>>> I was chatting with Jorn earlier, and he was thinking of something that
>>> I tried to implement at some point, but then I gave up; basically, the big
>>> issues with withOwnerThread is that (1) must be called by owner thread and
>>> (2) the owner thread must know the thread picking it up.
>>>
>>> It is in principle possible to break up withOwnerThread in two halves:
>>> e.g. detach (removes ownership) +  attach (re-adds ownership to a detached
>>> segment).
>>>
>>> So, if in your use cases, the workflow is clear enough that you know
>>> _when_ the segment is no longer being worked on, the owner thread could
>>> simply call something like MemorySegment::detach (and maybe put the segment
>>> on a queue); later on, another thread (a cleaner one?) might pick it up,
>>> attach to it, and call close.
>>>
>>> Is this something that would help in the use cases being considered in
>>> this thread? Or does it still fall in the "too restrictive" category? My
>>> sense when speaking with Uwe at the committer workshop in January was that,
>>> at least for Lucene, this fell in the latter bucket (e.g. not enough).
>>>
>>> Maurizio
>>>
>>>
>>> Kind regards
>>> Johanmes
>>>
>>> Maurizio Cimadamore <maurizio.cimadamore at oracle.com> schrieb am Mi.,
>>> 22. Juli 2020, 11:51:
>>>
>>>>
>>>> On 22/07/2020 00:44, Johannes Lichtenberger wrote:
>>>>
>>>> So, it likely will not be part of Java 15, right?
>>>>
>>>> Right.
>>>>
>>>>
>>>> I'm running into the same problem, when I'm sharing a transaction (only
>>>> one thread ever accesses it) with another thread and the other thread
>>>> closes the transaction. Same problem with the segment close()-method.
>>>>
>>>> Are you too also using mapped segments? Or just plain native segments
>>>> allocated with MemorySegment::allocateNative?
>>>>
>>>> Thanks
>>>> Maurizio
>>>>
>>>>
>>>> kind regards
>>>> Johannes
>>>>
>>>> Am Mi., 22. Juli 2020 um 01:24 Uhr schrieb Maurizio Cimadamore <
>>>> maurizio.cimadamore at oracle.com>:
>>>>
>>>>>
>>>>> On 21/07/2020 22:51, NekoCaffeine wrote:
>>>>> >>
>>>>> https://gist.github.com/mcimadamore/128ee904157bb6c729a10596e69edffd
>>>>> > This is a great example.
>>>>> >
>>>>> >> The *right* solution from an API perspective is to find a way to
>>>>> disable confinement in a way that works
>>>>> > I very much agree with this point, which is exactly what I want to
>>>>> express.
>>>>> >
>>>>> > So from a design rather than implementation perspective, should I
>>>>> expect a new way to disable thread checking or should I use
>>>>> CustomMappedSegment?
>>>>>
>>>>> So the crux of the issue here is that there's no Runnable that will
>>>>> unmap a given mapped address. But such a Runnable can be implemented
>>>>> in
>>>>> several ways:
>>>>>
>>>>> * with plain JNI
>>>>> * using the ForeignLinker API (although that is not part of JDK 15 -
>>>>> but
>>>>> is available in the Panama repo)
>>>>> * going all in and re-implement mapped segment (similar to what I've
>>>>> done with CustomMappedSegment)
>>>>>
>>>>> I think from an API perspective, if we manage to solve the confinement
>>>>> problem, then I expect the memory segment API will have an extra
>>>>> method
>>>>> to either turn a confined segment into an unconfined one, or
>>>>> vice-versa.
>>>>> So you will be able to just create your mapped segment using
>>>>> mapFromPath
>>>>> and then just 'share' it (and make it unconfined).
>>>>>
>>>>> Hope this helps.
>>>>>
>>>>> Maurizio
>>>>>
>>>>> >
>>>>> > Thanks,
>>>>> >
>>>>> > NekoCaffeine
>>>>>
>>>>