On 24 Nov 2003, at 17:42, Oliver Zeigermann wrote:
Stefano Mazzocchi wrote:
On 24 Nov 2003, at 09:59, Oliver Zeigermann wrote:
Stefano Mazzocchi wrote:
On 19 Nov 2003, at 05:03, Oliver Zeigermann wrote:
Hi Stefano,
I am wondering why you check for any lock for the source uri. Also, why no lock for the destination? Why is there no patch for the copy method?
Could you please explain a little bit more?!
Glad you asked ;-)
That bug is *really* subdle and was hard to find. It is basically triggered by the fact that the "move" operation performed between two different stores is not atomic, but it done as "copy" then "delete". But "copy" doesn't (nor should, IMHO) check for the lock of the source uri, but "delete" obviously does. This transforms the "move" of a locked resource a "copy", which is wrong (the litmus tests fail for this reason).
Hmmm. Do not understand... Aren't you mixing up isolation of a transaction with general locking? There are those locks set externally spanning multiple requests and there *might* be internal locks guaranteeing correctness of single requests.
I choose to fix it by checking for the lock on the source URI right away to avoid even trying to do the move if the source URI is locked. This solves the issue cleanly and it's store independent.
Hmmm, wondering where this is done for the Delete method, really could not find it. I still think the more appropriate place for this check should be MacroImpl to use this functionality from other tiers as well.
The only thing I care about is to pass the tests. I don't care how it gets fixed. I just proposed a solution that worked for me, if you think you have a better solution that fixes the problem, I'm all for it.
The offending code is in MacroImpl, in fact.
public void move(SlideToken token, String sourceUri,
String destinationUri, MacroParameters parameters,
CopyRouteRedirector copyRedirector, CopyListener copyListener,
DeleteTargetRedirector deleteRedirector, DeleteListener deleteListener)
throws CopyMacroException, DeleteMacroException {
if (Configuration.useBinding(namespace.getUri(token, sourceUri).getStore()) &&
Configuration.useBinding(namespace.getUri(token, destinationUri).getStore()) &&
sameStore(token, sourceUri, destinationUri)) {
rebind(token, sourceUri, destinationUri, parameters,
copyRedirector, copyListener, deleteRedirector, deleteListener);
using "rebind" is no problem (but for some reason this is not executed even on the same store!)
This is currently disabled as it requires a store implementation different from the current :(
I see.
}
else {
copy(token, sourceUri, destinationUri, parameters,
copyRedirector, copyListener, deleteRedirector, deleteListener);
delete(token, sourceUri, parameters, deleteRedirector, deleteListener);
}
}
the problem is here! since the first copy happens with no problems and the delete fails before of locking, leaving a orphaned copy and exiting with the wrong error code.
Seeing it this way. Why is there no high level lock check for DeleteMethod?
Very good question :-)
I don't know.
My philosophy for patching is: change the smallest possible amount of lines. it follows incremental XP practices and reduces potential issues.
Delete works, so I didn't patch it.
It actually works, as in MacroImpl it tries to acquirs locks which fails, so delete fails. It works, but as I said, I would like to have a general revision for 2.1 :)
Oliver
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
