[WW-5312] Attempt to fix ExecuteAndWaitInterceptor inconsistent processing #688
Conversation
- Improve ExecuteAndWaitInterceptor state behaviour, added debug and trace logging. - Ensure StrutsBackgroundProcess thread done state always set on completion. - Fix SessionMap processing issue caused by divergence of method signatures, which can result in the ancestor methods getting called (bypassing actual session processing). - Update SessionMapTest to correspond to changes, add tests to try and detect some (put/remove) method signature behaviour changes.
|
Hello Apache Struts Team. The commit summary basically covers what is being attempted in the PR. Other than some minor cleanup (in the modules where changes were made), the changes were an attempt to fix the issue found with WW-5312. The regression tests pass (locally), as well as interactive testing using the Struts2 Showcase application, but the changes might still need improvements. The Please review and advise if the PR looks OK, and if there are any recommended changes. Note: It looks like there was a "sonar-maven-plugin" failure for the Java 11 build environment run, but the build itself seems to have completed OK. Possibly an issue with the automated build ? |
| try { | ||
| executor.shutdown(); | ||
| } finally { | ||
| super.destroy(); | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks a lot for your comment, yet I was thinking about redesigning the current *Map approach (there is ApplicationMap, RequestMap as well) and use native objects to have more control over them. Right now we must prevent extra cuotion to secure the framweork (excluded #application or #request in ParametersInterceptor).
Anyway this will happen in Struts 7.0 at leas :)
Update: