[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a diff in pull request #688: [WW-5312] Attempt to fix ExecuteAndWaitInterceptor inconsistent processing

["JCgH4164838Gh792C124B5 (via GitHub)" <gi...@apache.org>]

JCgH4164838Gh792C124B5 commented on code in PR #688:
URL: https://github.com/apache/struts/pull/688#discussion_r1208111764


##########
core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java:
##########
@@ -394,7 +404,10 @@ public void init() {
 
     @Override
     public void destroy() {
-        super.destroy();
-        executor.shutdown();
+        try {
+          executor.shutdown();
+        } finally {
+          super.destroy();
+        }

Review Comment:
   Hi Lukasz.  Thanks for reviewing the proposed fix changes.  😄 
   
   Looking at the commit history, `SessionMap`  has extended `AbstractMap` since its earliest commits (many, many years ago).  Since `SessionMap` is _public_ and has been structured a certain way for so long, changing its design to delegate (to a non-accessible internal map, instead of extending) could have unexpected impacts (especially for anything that might try to use/extend it in user-space code).
   
   Staying with the long-standing (preexisting) design might be safer in terms of potential side-effects.  Keeping `@overrides` annotations in place along with some unit tests that attempt to detect breaking-changes should help.  Considering a `SessionMap` redesign would probably need to be a standalone issue/work item, if people think it is needed (taking into account the existing design seems to have worked sufficiently for a long time).
   
   Hopefully my reasoning above makes sense.  If it seems like other changes/improvements should be made to the PR, please let me know.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org