[osmosis-dev] calling DataPostbox.release() multiple times

[Brett Henderson]

On 16 January 2012 07:22, Richard Hansen <rhansen at bbn.com> wrote:

> On 2012-01-15 07:34, Brett Henderson wrote:
>
>> You're correct in saying that Releasable classes can be released
>> multiple times, however that is not typically how they're used in the
>> Osmosis pipeline.
>>
>
> Does it make sense to change the Releasable API definition to say that
> release can only be called once?  Or might that break multiple-sink stages
> like merge?


I've updated the Releasable API definition to say that implementations
should support release being called multiple times but that it isn't
mandatory and cannot be relied on by clients.

Multiple sink stages like merge expose two separate sink instances to the
two upstream pipes (through MultiSink interface which allows multiple Sink
instances to be obtained) which feed into separate DataPostbox instances
internally.  Each sink instance only has its release method called once.
So a single release call limitation shouldn't break anything.


>
>  Looking at the code a bit more I think it might only need minor changes.
>>  I had thought that release() and outputRelease() changed the state to
>> equal that of a freshly constructed DataPostbox (to support reuse?), but
>> that's not true:  inputExit and outputExit are false after construction but
>> true after release().  So simply checking inputExit at the beginning of
>> release() should take care of it.  See the attached (untested) patch.
>>
>
release() and outputRelease() have been designed to support re-use.  I
don't think I've tested it, but I believe it will work.  The two variables
you mention are initialized near the start of those methods and only read
after the xxxxReleased flags have been set so their initial state isn't
important.  However I can see that is confusing so I've modified their
initial state to match their final state of true (I'll check it in shortly).

Your patch would probably work but I'd prefer to continue to support re-use
rather than to use that variable for a second purpose.


>
> An alternative is to increase the burden on the users of DataPostbox. They
> could be required to either ensure that the output thread restarts or avoid
> calling DataPostbox.release() once the output thread exits. For example,
> EntityBuffer.run() could set an 'outputThreadIsRunning' boolean at the top
> and clear it at the end.  Then EntityBuffer.release() would be modified to
> only call buffer.release() if outputThreadIsRunning is true.


Race conditions between threads are tough to avoid, and I believe the above
change would introduce one.  If the output thread hasn't entered
EntityBuffer.run yet and the outputThreadIsRunning flag hasn't been set,
but the calling thread encounters an early exception and calls
EntityBuffer.release, the calling thread will think that the output thread
isn't running and there is no need to perform release functionality.  This
would then lead to the output thread blocking when it finally does start.

The way the class is constructed now, each thread will synchronise at the
initialize, complete and release methods.  If either thread skips straight
to release then the other thread detects that as an error condition and
aborts appropriately, but in all cases both threads will synchronise around
release before exiting.  The only reason to synchronise around release is
to support re-use which may never be used in practice but it should work
well.  We can't even rely on the initialize methods being called because it
is possible for a thread to fail before that is called.

I'd rather leave the class as it is designed now.  It's a tradeoff between
supporting re-use and incurring the limitation that every call to release
must be matched by a call to outputRelease in another thread.  I don't
think it's possible to have it both ways.

Brett
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/osmosis-dev/attachments/20120118/b3540370/attachment.html>