Skip to content

Conversation

yasserzamani
Copy link
Member

Currently we won't support exec and wait from de-serialized session and maybe add this support some day on user demand. Why I think to drop such support? It's not a good practice to try serializing such large or variant unpredictable objects like action and invocation (CWE-579: J2EE Bad Practices: Non-serializable Object Stored in Session).

Copy link
Member

@lukaszlenart lukaszlenart left a comment

Choose a reason for hiding this comment

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

Left few comments

@@ -44,9 +45,8 @@
* @param invocation The action invocation
* @param threadPriority The thread priority
*/
public BackgroundProcess(String threadName, final ActionInvocation invocation, int threadPriority) {
BackgroundProcess(String threadName, final ActionInvocation invocation, int threadPriority) {
Copy link
Member

Choose a reason for hiding this comment

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

In such case you are reducing re-usability of this class in other interceptors, user will have to copy this class if she wants to re-implement the interceptor

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks 👍 I did not know user may want. fixed :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... this is a bit different than development of a normal app. This a framework that should be easily extendable by users and quite often we need to break good practices (like reducing scope) because users would like to use it. You wasn't the first one who did that ;-)

@@ -75,7 +75,7 @@ public void run() {
*
* @throws Exception any exception thrown will be thrown, in turn, by the ExecuteAndWaitInterceptor
*/
protected void beforeInvocation() throws Exception {
private void beforeInvocation() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Reducing scope can affect users and blocks adding other implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks 👍 . fixed :)

@@ -86,7 +86,7 @@ protected void beforeInvocation() throws Exception {
*
* @throws Exception any exception thrown will be thrown, in turn, by the ExecuteAndWaitInterceptor
*/
protected void afterInvocation() throws Exception {
private void afterInvocation() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, using protected allows users re-implementing this class easily

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks 👍 . fixed :)

@coveralls
Copy link

coveralls commented Dec 13, 2017

Coverage Status

Changes Unknown when pulling dda3fac on yasserzamani:WW-4900 into ** on apache:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6b13155 on yasserzamani:WW-4900 into ** on apache:master**.

@lukaszlenart
Copy link
Member

testSerializeDeserialize of BackgroundProcessTest is failing ...

@yasserzamani
Copy link
Member Author

Was a good catch by junit 👍 I should have wait for background thread. fixed. thanks!

@coveralls
Copy link

coveralls commented Dec 13, 2017

Coverage Status

Changes Unknown when pulling a8ecd9b on yasserzamani:WW-4900 into ** on apache:master**.

@lukaszlenart
Copy link
Member

Let's rock 👍

@lukaszlenart lukaszlenart merged commit d4b620d into apache:master Dec 13, 2017
@yasserzamani yasserzamani deleted the WW-4900 branch December 15, 2017 09:07
@tiztm
Copy link

tiztm commented Aug 17, 2018

I think so , such huge object in session is too bad
It seems that,TokenSession needs some changes too.

@yasserzamani
Copy link
Member Author

@tiztm , AFAIK token is just a simple string and shouldn't have any problem with serialize then de-serialize. Or maybe I didn't get something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants