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.

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

Cancellation handler #126

Merged
merged 6 commits into from Oct 25, 2017
Merged

Conversation

aalmiray
Copy link
Contributor

@aalmiray aalmiray commented Oct 20, 2017

As discussed during BaselOne, DeferredFutureTask is now able to cleanup any resources when the underlying task is interrupted or cancelled.
Tests were parameterized with JUnitParams,


This change is Reviewable

@aalmiray aalmiray added this to the 2.0 milestone Oct 20, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 41.871% when pulling d798fa9 on aalmiray:cancellation-handler into 01d4567 on jdeferred:2.x.

@saturnism
Copy link
Member

Reviewed 5 of 6 files at r1.
Review status: 5 of 6 files reviewed at latest revision, 2 unresolved discussions.


subprojects/jdeferred-core/src/main/java/org/jdeferred/DeferredFutureTask.java, line 166 at r1 (raw file):

		} catch (InterruptedException e) {
			// TODO: should cleanup be called here?
			cleanup();

this is a tough one.. what should be the right behavior?
should pass the cancelation cause (the exception) into the cancellation handler?


subprojects/jdeferred-core/src/main/java/org/jdeferred/DeferredFutureTask.java, line 175 at r1 (raw file):

		} catch (Throwable t) {
			// TODO: this exception will be lost. Handle it via global ExceptionHandler?
			t.printStackTrace();

should log the error here. let's add a TODO to forward to global handling when we have it.


Comments from Reviewable

@aalmiray
Copy link
Contributor Author

Review status: 4 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


subprojects/jdeferred-core/src/main/java/org/jdeferred/DeferredFutureTask.java, line 166 at r1 (raw file):

Previously, saturnism (Ray Tsang) wrote…

this is a tough one.. what should be the right behavior?
should pass the cancelation cause (the exception) into the cancellation handler?

Indeed. Decided to pass the exception/cause as this case is triggered when the executing thread is interrupted from the outside the task


subprojects/jdeferred-core/src/main/java/org/jdeferred/DeferredFutureTask.java, line 175 at r1 (raw file):

Previously, saturnism (Ray Tsang) wrote…

should log the error here. let's add a TODO to forward to global handling when we have it.

done.


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 41.898% when pulling 4563bb9 on aalmiray:cancellation-handler into 666c4dd on jdeferred:2.x.

@saturnism
Copy link
Member

Reviewed 1 of 6 files at r1.
Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.


subprojects/jdeferred-core/src/main/java/org/jdeferred/DeferredFutureTask.java, line 175 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

done.

still see t.printStackTrace(). That should be removed now that we have the log entry, right?


Comments from Reviewable

@aalmiray
Copy link
Contributor Author

Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.


subprojects/jdeferred-core/src/main/java/org/jdeferred/DeferredFutureTask.java, line 175 at r1 (raw file):

Previously, saturnism (Ray Tsang) wrote…

still see t.printStackTrace(). That should be removed now that we have the log entry, right?

fixed


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 41.879% when pulling 77b1f0e on aalmiray:cancellation-handler into 666c4dd on jdeferred:2.x.

@saturnism
Copy link
Member

:lgtm:


Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@saturnism saturnism merged commit f3fbe2e into jdeferred:2.x Oct 25, 2017
@aalmiray aalmiray deleted the cancellation-handler branch October 27, 2017 14:17
@saturnism saturnism mentioned this pull request Oct 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants