Community
Participate
Working Groups
I watched as someone was trying to publish a small app (~60 files) for the first time and the publishing job 'hung' for over a minute, then failed with ~60 error messages starting with "Could not replace with temp file". After debugging for a while, we fould that PublishHelper.publishFull() was being called to copy the module's contents into a folder, but a file with the same name as the folder already existed in the target folder. PublishHelper would walk through every file in the module, copy it into a temp folder, fail when trying to create the parent folder to move the new file into, retry 10 times (pausing 100ms between each try), then fail and move on to the next file. Marking this as major, since there are no checks on monitor.isCanceled() between each file, and hence in a large app (say 1000+ files) this would hang Eclipse for 1000s (over 15 min). The core problem is that PublishHelper ignores the result of File.mkdirs() in several places. isCanceled() should also be called, and I think the number of retries/pauses should be shortened, since although 1s is short, there can be many files within an application.
Created attachment 225737 [details] v1.0
The changes look good. Using Tomcat 7.0, I published a dynamic web project. To hit the code, I use "clean" to call the method org.eclipse.wst.server.core.util.PublishHelper.publishFull(IModuleResource[], IPath, IProgressMonitor), as a normal publish does not hit it. 1. With debug breakpoint in org.eclipse.wst.server.core.util.PublishHelper.copy(IModuleResource, IPath, IProgressMonitor), I cancel the copy to ensure the monitor.isCanceled check will cause the copy to be cancelled 2. I hacked in a case where f.mkdirs fails in org.eclipse.wst.server.core.util.PublishHelper.copy(IModuleResource, IPath, IProgressMonitor), to ensure that copyFile is not called (which can cause the hang) and verified that an error is returned immediately 3. I tested a simple publish, update, and clean with a servlet and HTML to ensure that the smoke test scenario was not broken
Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. This problem causes publish to hang in some cases when dealing with large application. Is there a work-around? If so, why do you believe the work-around is insufficient? There is no good workaround given that the existing code does not check for the progress monitor cancel. Therefore, the user have to wait forever. How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? Tested using on tomcat manually to make sure the publish clean scenario is still working fine (the described code will be executed during a publish clean). Give a brief technical overview. Who has reviewed this fix? Adding monitor cancel check in various places. Also, check to make use of the result from mkdirs() and an error status instead of continue to copy when the parent directory failed to be created. Reviewed by Steven Hung. What is the risk associated with this fix? Relatively low since the code affects the publish full and publish clean operation only which is not a very frequent operation (comparing to delta publish). Also, the code change is fairly straight forward.
I assume the initial publish will hit this code, so a fairly high risk area to make changes. That said, the changes look straight forward, and assuming adequate testing and review, seems like a good bug to fix. I recommend other PMC members review this fix, but will approve for now.
The initial publish does not call this. This method will only get called when someone do a clean publish from the GUI or some adopter explicitly calls the publishFull() API.
Patch looks good - thanks
Code released to 3.5
Changes released 3.4.2.
New Gerrit change created: https://git.eclipse.org/r/109149
New Gerrit change created: https://git.eclipse.org/r/109148
