Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

for #196 - custom UserAgent / PdfBoxUserAgent #198

Closed
wants to merge 1 commit into from
Closed

for #196 - custom UserAgent / PdfBoxUserAgent #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public class PdfBoxRenderer implements Closeable {
_outputDevice = new PdfBoxOutputDevice(DEFAULT_DOTS_PER_POINT, testMode);
_outputDevice.setWriter(_pdfDoc);

PdfBoxUserAgent userAgent = new PdfBoxUserAgent(_outputDevice);
PdfBoxUserAgent userAgent = PdfBoxUserAgentFactory.getPdfBoxUserAgent(_outputDevice);

if (httpStreamFactory != null) {
userAgent.setHttpStreamFactory(httpStreamFactory);
Expand Down
95 changes: 95 additions & 0 deletions openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxUserAgentFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package com.openhtmltopdf.pdfboxout;

/**
* PdfBoxUserAgentFactory. Singleton.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no! Please no singletons....

Copy link
Author

@delafer delafer Apr 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Singleton & Multiton patterns appeared a long before Guice, Spring iOC, Weld, Dagger, etc.
"Singletons should only be @singleton annotated and injected classes."
who has said such a stupid thing?
Singletons should only be @singleton annotated in CDI / DI Enviroments, where dependency injection container is used, without CDI container, you can still use and write Singletons and where are normal classes which have static fields-like behavior.

"does not fit the current design"
LOL. I do not see any design at all. (PdfBoxRenderer = 18 args constructor + 108 lines of code in constructor init method. wonderful design. I am working for about 20 years as developer and have rarely seen such a wonderful design)

public class PdfBoxUserAgentFactory {

private static class LazyFactoryHolder {
private static final PdfBoxUserAgentFactory INSTANCE = new PdfBoxUserAgentFactory();
}

protected static PdfBoxUserAgentFactory instance() {
return LazyFactoryHolder.INSTANCE;
}

private static class LazyHolder {
private static final UserAgentFactory INSTANCE = new UserAgentFactory() {
public PdfBoxUserAgent getPdfBoxUserAgent(PdfBoxOutputDevice outputDevice) {
return new PdfBoxUserAgent(outputDevice);
}
};
}

/*
* In all versions of Java, the idiom enables a safe, highly concurrent lazy initialization with good performance.
*/
private static UserAgentFactory factoryDef() {
return LazyHolder.INSTANCE;
}

private ThreadLocal<UserAgentFactory> threadLocal;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This smells like memory / class loader leaks in Tomcat etc. on WAR unloading... ThreadLocals are hard to do right in a servlet context.

Copy link
Author

@delafer delafer Apr 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never had any problems with ThreadLocal clases either Tomcat, Jetty, OC4J, Wildfly, Weblogic, etc. I can list / enumerate dozens of well-known / opensource projects using this standard Java class. It's available since JAVA 1.2 ;-) ThreadLocal could lead to memory leaks, but only if you don't know how to use it.

If only this library (openhtmltopdf) was developed using a thoughtful design it would not be necessary at all to create something like this. I almost threw up as I have seen constructor with 18 arguments and constructor itself has 108 lines of code! schoolchildren writing better code.

private UserAgentFactory global;


{
threadLocal = new ThreadLocal<UserAgentFactory>();
}

/**
* Register [global] own / custom PdfBoxUserAgentFactory
* @param factory - implementation of UserAgentFactory interface
*/
public void registerGlobalPdfBoxUserAgentFactory(UserAgentFactory factory) {
this.global = factory;
}

/**
* UnRegister [global] own / custom PdfBoxUserAgentFactory
*/
public void unregisterGlobalPdfBoxUserAgentFactory() {
this.global = null;
}

/**
* Register [threadLocal] own / custom PdfBoxUserAgentFactory
* @param factory - implementation of UserAgentFactory interface
*/
public void registerPdfBoxUserAgentFactory(UserAgentFactory factory) {
threadLocal.set(factory);
}

/**
* UnRegister [threadLocal] own / custom PdfBoxUserAgentFactory
*/
public void unregisterPdfBoxUserAgentFactory() {
threadLocal.remove();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you forget this, you will have a class loader leak in your servlet container on unload....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just like with streams, if you opened it - you must close it. It is not an argument not to use them at all. Tomcat trying to reuse user threads, which could lead to problems with threadlocal ,what why tomcat developers recommends to clean them manually. Anyway tomcat was never a good choice for a production use in a serious projects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library can / will be used in whatever context the users want. Also it might be used with Tomcat (or Jetty, which also reuses threads). If we already know that using ThreadLocals will cause problems if the user is not careful we should avoid it - especially in the public API. The goal should always be to design an API in such a way that the user can not shoot himself into the foot. Not everyone has the time / is willing to read the documentation carefully. Being surprised by some "out of PermGen" errors in production is not so funny - especially if you don't have any clue where this could come from, as you usually have may different libraries in use.

}

/**
*
* @param outputDevice - instance of PdfBoxOutputDevice, mandatory field
* @return instanceof PdfBoxUserAgent
*/
public static PdfBoxUserAgent getPdfBoxUserAgent(PdfBoxOutputDevice outputDevice) {
return instance().getPdfBoxUserAgent0(outputDevice);
}


private PdfBoxUserAgent getPdfBoxUserAgent0(PdfBoxOutputDevice outputDevice) {
UserAgentFactory thLocal = threadLocal.get();
return (thLocal != null ? thLocal : global != null ? global : factoryDef()).getPdfBoxUserAgent(outputDevice);
}


public interface UserAgentFactory {
/**
* Factory to use custom implementation of PdfBoxUserAgent classes
* @param outputDevice
* @return
*/
public PdfBoxUserAgent getPdfBoxUserAgent(PdfBoxOutputDevice outputDevice);
}


}
9 changes: 9 additions & 0 deletions openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfRendererBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ public void run() throws Exception {
}
}

/**
* Returns an instance of PdfBoxUserAgentFactory in case you<br/>
* need to use custom implementation of {@link PdfBoxUserAgent} class
* @return PdfBoxUserAgentFactory instance
*/
public PdfBoxUserAgentFactory getPdfBoxUserAgentFactory() {
return PdfBoxUserAgentFactory.instance();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats bad API design. It also breaks the builder contract (i.e. everything but build() always returns PdfRendererBuilder for method chaining). Better would be

  • Just define the UserAgentFactory interface (as above)
  • The builder should have a field userAgentFactory with a default implementation assign i.e.
        _userAgentFactory = new UserAgentFactory() {
            public PdfBoxUserAgent getPdfBoxUserAgent(PdfBoxOutputDevice outputDevice) {
                return new PdfBoxUserAgent(outputDevice);
            }
        };
  • A setter to override the default implementation. i.e.
public PdfRendererBuilder setUserAgentFactory(UserAgentFactory customFactory) {
  this._userAgentFactory = customFactory;
  return this;
}

Of course it would require an additional parameter to the PdfBoxRenderer....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a suspicion that you just did not understand the code above.
"build() always returns PdfRendererBuilder for method chaining"
Of course it returns always a PdfRendererBuilder, it should be used in this manner:

    PdfBoxRenderer renderer = null;
    try {
        builder.getPdfBoxUserAgentFactory().registerGlobalPdfBoxUserAgentFactory(new PdfBoxUserAgentFactory.UserAgentFactory() {
            @Override
            public PdfBoxUserAgent getPdfBoxUserAgent(PdfBoxOutputDevice pdfBoxOutputDevice) {
                return new ExtendedPdfBoxUserAgent(pdfBoxOutputDevice);
            }
        });
        renderer = builder.buildPdfRenderer();
        //customUserAgent(renderer);
        renderer.layout();
        renderer.createPDF();
    } finally {
        if (renderer != null)
            renderer.close();
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@delafer Sorry, but where is the unregisterPdfBoxUserAgentFactory() call? Oh, you forgot it. So a class loader leak is incoming on WAR reload ... This is what I mean by bad API design, if it's easy to forget something you will forget to do it... and the problem will only manifest after the n-th reload of the WAR..

Yes, creating an idiot proof API is hard, but should be the goal never the less.

Method chaining means: builder.setXY().setAbc().setWhatever().build(). For another example look here: https://google.github.io/gson/apidocs/com/google/gson/GsonBuilder.html

}

/**
* Build a PdfBoxRenderer for further customization. Remember to call
* {@link PdfBoxRenderer#cleanup()} after use.
Expand Down