This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
| Summary: | MultiDataObject.handleCopy() should copy secondary files before the primary | ||
|---|---|---|---|
| Product: | platform | Reporter: | Andrew Sherman <asherman> |
| Component: | Data Systems | Assignee: | David Konecny <dkonecny> |
| Status: | VERIFIED FIXED | ||
| Severity: | blocker | CC: | jtulach, ttran |
| Priority: | P3 | ||
| Version: | 3.x | ||
| Hardware: | All | ||
| OS: | All | ||
| Issue Type: | DEFECT | Exception Reporter: | |
| Attachments: | diffs without line wraps | ||
Created attachment 10159 [details]
diffs without line wraps
Yarda, I would appreciate your opinion on this. Andrew's proposed solution make sense to me, but I can imagine that this change can also break something else. Btw. how is that possible that DO recognition starts before the handleCopy is finished? The copying is done in atomic operation. Is not problem here? Other possibility would be to override handleCopy method only in the DO which creates secondary files. It is not common I think that secondary files are generated if not found. 1. solution makes sence, but can break something. As far as I can imagine the breakage shall not be too big. At worse there will be two data objects: one for a secondary entry and one for a primary file containing also the above secondary entry 2. DO recognition starts as soon as somebody calls DataObject.find. This is triggered by FileEvents (prevented by atomic action) but also by direct calls to that method (not prevented by anything) 3. Solving the problem in the actual data object is possible and allows synchronization between the file creation and file recognition. The DataLoader may block when recognizing a file during copying of a data object of the same type. Overall, I think the change of copy order is a good thing. Thanx Yarda. Andrew, you marked this P2. Sounds like it is serious problem for NB35? Do you think it should be fixed for NB35? Btw. is it still possible? I think we just passed some milestone and it is not possible anymore. But I might be wrong. Anyway, I propose to fix it as you suggested but for next version (4.0). I will do it early next week. As for the NB35 I would be very hesitant to fix it because of possibility to break something else. If it is really annoying problem I would propose short term fix for NB35 directly in DO implementation which causes this problem. Let me know your opinion. Thanx. > Btw. how is that possible that DO recognition > starts before the handleCopy is finished? > The copying is done in atomic operation. Is > not problem here? } There is a subtlety here that I didn't mention before. The bug only happens when you copy and paste two or more objects. The thread that gets in a race with the copy is (I think) the thread that copied the first object as it fires events outside of the lock. To be clear the stack trace that is doing the refresh looks like this: loaders.FolderList.refresh(FolderList.java:285) loaders.FolderList.fileChanged(FolderList.java:359) util.WeakListener$FileChange.fileChanged(WeakListener.java:566) filesystems.FCLSupport.dispatchEvent(FCLSupport.java:67) filesystems.FileObject$ED.dispatch(FileObject.java:719) filesystems.EventControl.invokeDispatchers(EventControl.java:160) filesystems.EventControl.exitAtomicAction(EventControl.java:138) filesystems.EventControl.runAtomicAction(EventControl.java:91) filesystems.FileSystem.runAtomicAction(FileSystem.java:414) loaders.DataObject.copy(DataObject.java:505) loaders.DataFolder$6.handlePaste(DataFolder.java:1330) loaders.DataTransferSupport$PasteTypeExt.paste(DataTransferSupport.java:114) actions.PasteAction.executePasteType(PasteAction.java:183) actions.PasteAction.access$100(PasteAction.java:60) actions.PasteAction$ActionPT.actionPerformed(PasteAction.java:777) .core.ModuleActions$1.run(ModuleActions.java:97) util.Task.run(Task.java:136) util.RequestProcessor$Task.run(RequestProcessor.java:328) util.RequestProcessor$Processor.run(RequestProcessor.java:670) > Andrew, you marked this P2. Sounds like it is > serious problem for NB35? Do you think it should > be fixed for NB35? Btw. is it still > possible? I think we just passed some milestone > and it is not possible > anymore. But I might be wrong. I am not sure of the exact rules but clearly we are very near a release and there must be some risk in a fix of this kind, even if it makes sense. > Anyway, I propose to fix it as you suggested but > for next version (4.0). I will do it early next week. > As for the NB35 I would be very > hesitant to fix it because of possibility to break > something else. If it is really annoying problem I > would propose short term fix for NB35 > directly in DO implementation which causes this problem. That how I started out fixing this, a little messy as I had to copy more than just handleCopy down from MultiDataObject. But after I had made my fix I did some testing of another important DataObject and it showed a similar problem. And there may be other DataObjects that have the same problem. For S1S/J2EE group improved robustness of VCS has been a big priority and the way this bug appears is that the VCS support is flaky as you can't copy your user objects into VCS. Unfortunately this would be one of the first things user might do if they started to use VCS and first impressions are important. So I think this worth fixing in release35. I agree w/ David that there is significant risk in fixing this bug. (Andrew, you know why DataSystem needs to be redesigned). I prefer not to fix this bug in DS at this moment. It also seems to me the solution suggested by Andrew is more a hackaround than a clean fix. The order of primary/secondary files being copied is not defined, we may change the order but what if there are more than one secondary files? In which order should they be copied? Perhaps there is no such code which would be affected at this moment but without careful review we can't be sure. I don't rule out getting the suggested fix in release35 but we need to find the strategy to verify that the change does not break anything else. What parts of the module code might be affected? Yarda already indicated one potential breakage > 1. solution makes sence, but can break something. As far as I can > imagine the breakage shall not be too big. At worse there will be > two data objects: one for a secondary entry and one for a primary > file containing also the above secondary entry What does it mean from end user's point of view? Patch was integrated. Tests seemed to pass fine. Fixed in: Checking in src/org/openide/loaders/MultiDataObject.java new revision: 1.3; previous revision: 1.2 good that the fix is in trunk, BUT I still wonder what is the answer to my concern expressed in this report. What if later some module developer will ask us to revert the order to the other way? This is clearly a hack. David, please add a comment to the source file stating that it must be "secondary files first,then primary one", otherwise we'll break it again soon. It is already mentioned in the source file and there is also link to this issue. Yes, it is hack. It might be even useless now because Andrew had to fix this problem directly in his loaders for NB35. closed |
MultiDataObject.handleCopy() copies the primary file followed by the secondary files. This seems to cause problems on a slow filesystem such as vcsgeneric. The problems occur when the primary file is recognized by a loader and DataObject construction starts. Now the DataObject constructor may be initializing secondary files at the same time that handleCopy is copying them. After this handleCopy throws an exception when it tries to copy into a file that already exists. One way to fix this would be to have MultiDataObject.handleCopy() copy the secondary files before copying the primary file. Here is the code change I am suggesting (I moved things around so it would not wrap in a browser window. protected DataObject handleCopy (DataFolder df) throws IOException { FileObject fo; String suffix = existInFolder( getPrimaryEntry().getFile(), df.getPrimaryFile () ); if (suffix == null) throw new org.openide.util.UserCancelException(); - fo = getPrimaryEntry ().copy (df.getPrimaryFile (), - suffix); Iterator it = secondaryEntries().iterator(); while (it.hasNext ()) { ((Entry)it.next()).copy (df.getPrimaryFile (), suffix); } + fo = getPrimaryEntry ().copy (df.getPrimaryFile (), + suffix); try { return createMultiObject (fo); } catch (DataObjectExistsException ex) { return ex.getDataObject (); } }