8

The servlet spec (see my previous question) guarantees that the same thread will execute all Filters and the associated Servlet. Given this, I do not see any use for passing data using HttpServletRequest.setAttribute if there is the option to use a ThreadLocal (assuming you clean up properly). I feel that there are two benefits to using ThreadLocal: type-safety and better performance because no string keys or maps are being used (except probably into a thread collection by (non-string) thread id).

Could someone please confirm if I am right so I can proceed with abandoning setAttribute?

necromancer
  • 23,916
  • 22
  • 68
  • 115
  • 2
    Request attributes are, first and foremost, for JSP rendering. – Matt Ball Apr 10 '12 at 21:11
  • I am not using JSPs ("if there is the option to use a `ThreadLocal`"), so this question is regarding using them for other reasons. – necromancer Apr 10 '12 at 21:22
  • 1
    So where/how exactly are you looking to use these values? It _sounds_ like you're just using global-ish variables instead of DI or regular-ol-argument passing. – Matt Ball Apr 10 '12 at 21:24
  • I primarily need to transport the `User` and `EntityManager` objects from the user and database Filters to the Servlet. I also find that these are frequently and unexpectedly needed in code further down the line and I am tempted to use them well beyond the Servlet (i. e. in nested code called by doGet). I feel there may be a better way for deeper code - suggestions? But I do not feel there is a better way for Filter to Servlet - suggestions? I have no JSP/JSF/other-framework dependencies and I am using pure Java with Servlets and Filters. I control all the code. Thanks! – necromancer Apr 10 '12 at 21:31
  • 1
    `EntityManager` is _very_ commonly obtained with DI - you should seriously consider that approach. – Matt Ball Apr 11 '12 at 03:29
  • any pointers? (since i am not a DI expert) my first concern is that DI is just a technique which i can manually implement in my code -- if that makes sense would i use attributes or threadlocals? second concern, what is the lifetime of an entitymanager (i think that's worthy of a question so i'll post it). i have been using per-request entitymanagers, and if that is right, i am not sure what DI annotation specifies a per-request injection, and more importantly would object would get injected? thanks for your responses, btw. i appreciate your help. – necromancer Apr 11 '12 at 08:31

4 Answers4

3

thread local works better if you are trying to set "global" variables for code "behind" whatever is dealing with the HttpServletRequest. If you're using JSP/JSF pages, or some other web UI component that reads from HttpServletRequest, then you'll end up having to pull the information out of the ThreadLocal yourself. The 2 are not equivalent for most web programming.

Jim Barrows
  • 3,634
  • 1
  • 25
  • 36
  • Thanks. You do seem to imply `request.setAttribute` is better for the body of `doGet` or `doPost`, but I do not see why it is better to transport an object from a `Filter` to `doGet` using `setAttribute`/`getAttribute`? I feel that if the sender and receiver is entirely my own code with no dependencies then I should never use `setAttribute` even is the code is not "behind" whatever is dealing with HttpServletRequest. Am I missing something? – necromancer Apr 10 '12 at 21:27
  • If it's you're own code, it probably doesn't matter much. You'll find it difficult to integrate with other libraries and code however. – Jim Barrows Apr 10 '12 at 21:32
  • You say it "doesn't matter much". What about my thought that ThreadLocal is better: "benefits to using ThreadLocal: type-safety and better performance because no string keys or maps are being used"? I definitely will not need to integrate it, or if I would, it is sufficiently contained to change it at that point. – necromancer Apr 10 '12 at 21:35
  • Well, type safety is good. Performance, well, premature optimization is the root of all evil. Losing the ability to easily inter-operate in the future, not really worth the extra hassle in my opinion. – Jim Barrows Apr 10 '12 at 21:39
3

Using a ThreadLocal in this fashion implies your are relying on some type of singleton to maintain shared state. Ultimately, this is usually considered poor design since it has a number of drawbacks, including difficulty in encapsulating functionality, being aware of dependencies, and an inability to swap (or mock) out functionality.

The performance overhead of using attributes or a session is probably worth it as compared to dealing with the less usual proposal of ThreadLocal variables in a singleton. However, this of course depends on your specific use case and project. Using singletons in servlets as a way to maintain application state is somewhat common due to the difficulty in sharing application state with servlets (difficult to inject the dependencies), but it is unusual for those objects singleton objects to maintain per user state (outside of EJBs).

If using a session or setting a container object as an attribute, you would only be dealing with a single fetch via String (which would be O(1)), and then a single case to your container type (which that has specific accessors for all of the values you want). Calling code further down the line should take parameters as appropriate and avoid as best you can using any type of global or singleton.

Ultimately, before considering a somewhat unusual (although clever) solution in the name of performance, always test the straightforward implementation first to see if its performance is adequate. The 2ns that might be saved here are most likely insignificant compared to the 20+ms being spent on db queries and tcp connection establishment.

Trevor Freeman
  • 7,112
  • 2
  • 21
  • 40
  • I don't see the singleton. The object is per-thread for `ThreadLocal`, which has a 1-to-1 correspondence to per-request for `set/getAttribute`. The use-case is to transport the User object looked up by a Filter to the Servlet. In Filter I could say `setAttribute("user", user);` or `ThreadStore.setUser(user)`; and in Servlet, correspondingly: `User user = (User)getAttribute("user");` or `ThreadStore.getUser()` where ThreadStore is a convenience layer around ThreadLocal. I don't see the singleton. – necromancer Apr 10 '12 at 22:14
  • @agksmehx The singleton I am referring to would be the object or static class where you are holding the globally accessible ThreadLocal variable (it must exist in some global staticly accessible manner for you to use it as indicated). – Trevor Freeman Apr 10 '12 at 23:11
  • Doesn't the JVM hide all that from you? Please see http://docs.oracle.com/javase/7/docs/api/java/lang/ThreadLocal.html – necromancer Apr 10 '12 at 23:13
  • hmm... the example in the doc does have a singleton. i'm still not convinced if that's a bad thing or even if it is necessary. but your answer definitely earns my upvote :) – necromancer Apr 10 '12 at 23:16
  • @agksmehx Singletons are not necessarily bad (for instance, I do not think using them in a servlet is that bad since you can have difficulty passing in application state to a servlet otherwise), but they do have design implications (code using them is no longer encapsulated, thus it is harder to reuse). A ThreadLocal does not intrinsically require a singleton, but for you to be able to access this ThreadLocal from different classes without otherwise passing it in the constructor of the class or in the method being called, you must get it from something static (e.g. a singleton). – Trevor Freeman Apr 10 '12 at 23:26
  • Thanks - I want to accept your answer but I am not sure if "fetch via String" is indeed "O(1)" -- after all the string needs to be inspected character by character. Also, naturally, I feel your comment about singletons not necessarily bad in a servlet-like use-case should be part of the answer itself. – necromancer Apr 11 '12 at 00:16
  • @agksmehx The implementation underlying fetch by string in this case is a HashMap, which generally has O(1) performance. If your strings are constants (or at least not dynamically generated) then they will have their hash codes cached, so there will be no character by character inspection (lookup in the hashmap is by cached hash code). If your strings are dynamically generated (e.g. myString = "blah" + i + "foo" + count;) then you will incur the cost of hash code computation (but it should not be that bad unless you are doing this in a tight loop). I will update my answer about singletons. – Trevor Freeman Apr 11 '12 at 17:39
  • will the lookup in the hash code cache be based on the object id of the string? otherwise it seems like accessing the hash code cache itself would require string inspection. yes, in the case of transporting data from filter to servlet, the strings would be static. in fact, just a single string is needed to hold a compound object. also, i wish i could upvote your answer twice - i am still worried about the issue of correctly casting the returned servlet attribute. so it's not really about performance. this is a nuanced design decision. – necromancer Apr 11 '12 at 19:24
  • 1
    @agksmehx The String object itself caches its own hash code (and even String literals in quotes have an actual String object instance). What I would normally do in your case is have one object in the attributes or session that contains all the other objects, so I only have to worry about one cast. (e.g. UserContext context = (UserContext) request.getAttribute(USER_CONTEXT_KEY); int someInt = context.getSomeInt(); ). Deeper code then takes a UserContext object as a parameter (instead of passing request around or anything like that). Bonus points if UserContext is an interface. – Trevor Freeman Apr 11 '12 at 20:37
3

I'd advise against ThreadLocal.

I can see why you're considering it, but I think you're falling into the premature optimization trap. ThreadLocals are essentially global variables - rarely a good design. The time-savings will be negligible. I guarantee this will never be the bottleneck in your server's throughput.

Using ThreadLocal may also give you problems if you want to start using asynchronous responses to some requests (e.g. to support long-polling) as the threading model is quite different.

ᴇʟᴇvᴀтᴇ
  • 12,285
  • 4
  • 43
  • 66
  • type safety is my more important concern, and also switching from a string-keyed map to direct access is not in the category of premature optimization. kinda like moving from string representation of integers to byte representation would not be premature optimization. in summary, the main tradeoff is type-safety vs. "good design"(singleton, global, etc). the specific use case is transport data from filter to servlet and i am not able to see a concrete/specific demonstration of why it is not "good design". i appreciate good design but with code i give a bias to what fits specific use cases. – necromancer Apr 11 '12 at 19:12
3

Is ThreadLocal preferable to HttpServletRequest.setAttribute(“key”, “value”)?

Depends on the concrete functional requirement.

JSF, for example, stores the FacesContext in a ThreadLocal. This enables you to access all of the JSF artifacts, including the "raw" HttpServletRequest and HttpServletResponse anywhere in the code which is executed by the FacesServlet, such as managed beans. Most other Java based MVC frameworks follow the same example.

As per your comment,

I primarily need to transport the User and EntityManager objects from the user and database Filters to the Servlet. I also find that these are frequently and unexpectedly needed in code further down the line and I am tempted to use them well beyond the Servlet (i. e. in nested code called by doGet). I feel there may be a better way for deeper code - suggestions?

As to the User example for which I assume that this is a session attribute, I'd rather follow the same approach as JSF. Create a ThreadLocal<Context> where the Context is your custom wrapper class holding references to the current HttpServletRequest and maybe also HttpServletResponse so that you can access them anywhere down in your code. If necessary provide convenience methods to get among others the User directly from the Context class.

As to the EntityManager example, you could follow the same approach, but I'd personally not put it in the same ThreadLocal<Context>, but rather a different one. Or, better, just obtain it from JNDI in the service layer, which would allow you more finer grained control over transactions. In any case, please make absolutely sure that you handle the commit/close properly. Taking over the persistence and transaction management from the container should be done with extreme care. I'd really reconsider the aversion against using the existing and well-designed APIs/frameworks like EJB/JPA, otherwise you'll risk a complete waste of time of reinventing all the already-standardized APIs and stuffs.

See also:

Community
  • 1
  • 1
BalusC
  • 1,082,665
  • 372
  • 3,610
  • 3,555
  • Thank you for your answer. I prefer your approach, but for completeness, would you be kind enough to offer an opinion on the "singletons are bad" criticism of ThreadLocal? (all the other answers offer that criticism and prefer setAttribute so it would be nice to have that addressed if possible). thanks again! – necromancer Apr 12 '12 at 05:30
  • A `ThreadLocal` is not a singleton. A singleton is a static class with lazily loaded static content which always returns the same for the remnant of application's lifetime. – BalusC Apr 12 '12 at 12:48
  • Where you get the ThreadLocal from in this case is going to be a singleton (or otherwise static) class, since that is the mechanism of sharing the ThreadLocal between the filters and servlet. I would recommend not using such a singleton deeper in the code (unless you are passing an interface representation of the singleton down as a parameter). Furthermore, unless you are writing a framework which is encapsulating the underlying mechanisms, I would not use a ThreadLocal approach over attributes for performance reasons unless the application code is very tight such that this actually matters. – Trevor Freeman Apr 12 '12 at 17:43
  • @increment1 - this has been a learning experience. @BalusC is right that it is not a singleton (see the javadoc, you can create multiple instances, and there's no lazy loading). It is not a static class either in the sense that there are no instances. ThreadLocal static methods are factory methods and create multiple instances. See the source code for `get()`: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/lang/ThreadLocal.java There are no global/static data structures; it internally uses the non-public `threadLocals` member of the `Thread` class.(continued) – necromancer Apr 12 '12 at 19:30
  • So, no way there are any globals or singletons in the picture. There is already a per-thread `Thread` object available to any part of the code and `ThreadLocal` merely uses a field in per-thread `Thread` object to provide `ThreadLocal`s. Therefore, none of the criticisms of singletons or globals apply. Please let me know if I am wrong, and if not, I am sure you will consider this to be the best answer. Thanks again for your own answer. It definitely helped me dig deeper and understand things better. – necromancer Apr 12 '12 at 19:35
  • @BalusC, thank you for the awesome answer. I am pretty sure I will accept it soon! – necromancer Apr 12 '12 at 19:36
  • @agksmehx To clarify, no one is saying that a ThreadLocal is a singleton, but the ThreadLocal instance is retrieved / accessed by the servlets and filters either statically or via a singleton. I.e. How are you accessing the ThreadLocal (must be either via a static method or a singleton). – Trevor Freeman Apr 12 '12 at 23:39
  • @increment1 - no it is not via a singleton, it is a static method but static methods are perfectly fine. please look at the source code, it is interesting that there is no global variable being used! it just connects to the Thread object. And all your criticisms of singleton/global definitely do not apply in this case. – necromancer Apr 12 '12 at 23:42