Re: RFR: 8295324: JavaFX: Blank pages when printing [v3]
On Fri, 28 Oct 2022 13:54:40 GMT, eduardsdv wrote: >> This fixes a race condition between application and 'Print Job Thread' >> threads when printing. >> >> The race condition occurs when application thread calls `endJob()`, which in >> effect sets the `jobDone` flag to true, >> and when the 'Print Job Thread' thread was in the `synchronized` block in >> `waitForNextPage()` at that time. >> The 'Print Job Thread' thread checks `jobDone` flag after exiting the >> `synchronized` block and, if it is true, skips the last page. >> >> In this fix, not only the `jobDone` is checked, but also that there is no >> other page to be printed. >> It was also needed to introduce a new flag 'jobCanceled', to skip the page >> if the printing was canceled by 'cancelJob()'. > > eduardsdv has updated the pull request incrementally with two additional > commits since the last revision: > > - 8295324: Fix skipping of pages when printing > >This occurs in print(Graphics, PageFormat, int) if the 'jobDone' flag >was previously set by the 'endJob()' method. > - 8295324: Adjust the J2DPrinterJobTest > >The test now also checks for the second race condition around 'jobDone' >flag, which is in the print(Graphics, PageFormat, int) method. I think this is OK, but whilst I can see you went to some lengths to create a test case, I do not think it justifies the refactoring you had to do in order to create the test program. So I'd prefer that ALL refactoring be reverted, even if it means no test program. - PR: https://git.openjdk.org/jfx/pull/916
Re: RFR: 8295324: JavaFX: Blank pages when printing [v3]
On Fri, 28 Oct 2022 13:54:40 GMT, eduardsdv wrote: >> This fixes a race condition between application and 'Print Job Thread' >> threads when printing. >> >> The race condition occurs when application thread calls `endJob()`, which in >> effect sets the `jobDone` flag to true, >> and when the 'Print Job Thread' thread was in the `synchronized` block in >> `waitForNextPage()` at that time. >> The 'Print Job Thread' thread checks `jobDone` flag after exiting the >> `synchronized` block and, if it is true, skips the last page. >> >> In this fix, not only the `jobDone` is checked, but also that there is no >> other page to be printed. >> It was also needed to introduce a new flag 'jobCanceled', to skip the page >> if the printing was canceled by 'cancelJob()'. > > eduardsdv has updated the pull request incrementally with two additional > commits since the last revision: > > - 8295324: Fix skipping of pages when printing > >This occurs in print(Graphics, PageFormat, int) if the 'jobDone' flag >was previously set by the 'endJob()' method. > - 8295324: Adjust the J2DPrinterJobTest > >The test now also checks for the second race condition around 'jobDone' >flag, which is in the print(Graphics, PageFormat, int) method. Can someone else check this PR if @prace is not available? - PR: https://git.openjdk.org/jfx/pull/916
Re: RFR: 8295324: JavaFX: Blank pages when printing [v3]
On Fri, 28 Oct 2022 13:54:40 GMT, eduardsdv wrote: >> This fixes a race condition between application and 'Print Job Thread' >> threads when printing. >> >> The race condition occurs when application thread calls `endJob()`, which in >> effect sets the `jobDone` flag to true, >> and when the 'Print Job Thread' thread was in the `synchronized` block in >> `waitForNextPage()` at that time. >> The 'Print Job Thread' thread checks `jobDone` flag after exiting the >> `synchronized` block and, if it is true, skips the last page. >> >> In this fix, not only the `jobDone` is checked, but also that there is no >> other page to be printed. >> It was also needed to introduce a new flag 'jobCanceled', to skip the page >> if the printing was canceled by 'cancelJob()'. > > eduardsdv has updated the pull request incrementally with two additional > commits since the last revision: > > - 8295324: Fix skipping of pages when printing > >This occurs in print(Graphics, PageFormat, int) if the 'jobDone' flag >was previously set by the 'endJob()' method. > - 8295324: Adjust the J2DPrinterJobTest > >The test now also checks for the second race condition around 'jobDone' >flag, which is in the print(Graphics, PageFormat, int) method. @prrace Can you review this? - PR: https://git.openjdk.org/jfx/pull/916
Re: RFR: 8295324: JavaFX: Blank pages when printing [v3]
On Fri, 28 Oct 2022 13:54:40 GMT, eduardsdv wrote: >> This fixes a race condition between application and 'Print Job Thread' >> threads when printing. >> >> The race condition occurs when application thread calls `endJob()`, which in >> effect sets the `jobDone` flag to true, >> and when the 'Print Job Thread' thread was in the `synchronized` block in >> `waitForNextPage()` at that time. >> The 'Print Job Thread' thread checks `jobDone` flag after exiting the >> `synchronized` block and, if it is true, skips the last page. >> >> In this fix, not only the `jobDone` is checked, but also that there is no >> other page to be printed. >> It was also needed to introduce a new flag 'jobCanceled', to skip the page >> if the printing was canceled by 'cancelJob()'. > > eduardsdv has updated the pull request incrementally with two additional > commits since the last revision: > > - 8295324: Fix skipping of pages when printing > >This occurs in print(Graphics, PageFormat, int) if the 'jobDone' flag >was previously set by the 'endJob()' method. > - 8295324: Adjust the J2DPrinterJobTest > >The test now also checks for the second race condition around 'jobDone' >flag, which is in the print(Graphics, PageFormat, int) method. I found the second place with the race condition. It was in the print(Graphics g, PageFormat pf, int pageIndex) method. Also here "jobDone" was checked without checking if a page was pending. Please review. - PR: https://git.openjdk.org/jfx/pull/916
Re: RFR: 8295324: JavaFX: Blank pages when printing [v3]
> This fixes a race condition between application and 'Print Job Thread' > threads when printing. > > The race condition occurs when application thread calls `endJob()`, which in > effect sets the `jobDone` flag to true, > and when the 'Print Job Thread' thread was in the `synchronized` block in > `waitForNextPage()` at that time. > The 'Print Job Thread' thread checks `jobDone` flag after exiting the > `synchronized` block and, if it is true, skips the last page. > > In this fix, not only the `jobDone` is checked, but also that there is no > other page to be printed. > It was also needed to introduce a new flag 'jobCanceled', to skip the page if > the printing was canceled by 'cancelJob()'. eduardsdv has updated the pull request incrementally with two additional commits since the last revision: - 8295324: Fix skipping of pages when printing This occurs in print(Graphics, PageFormat, int) if the 'jobDone' flag was previously set by the 'endJob()' method. - 8295324: Adjust the J2DPrinterJobTest The test now also checks for the second race condition around 'jobDone' flag, which is in the print(Graphics, PageFormat, int) method. - Changes: - all: https://git.openjdk.org/jfx/pull/916/files - new: https://git.openjdk.org/jfx/pull/916/files/fdec73d8..acd4825b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=916=02 - incr: https://webrevs.openjdk.org/?repo=jfx=916=01-02 Stats: 85 lines in 2 files changed: 47 ins; 18 del; 20 mod Patch: https://git.openjdk.org/jfx/pull/916.diff Fetch: git fetch https://git.openjdk.org/jfx pull/916/head:pull/916 PR: https://git.openjdk.org/jfx/pull/916
