HttpSession listener ExtensionPoint #2303

Merged
merged 8 commits into from May 8, 2016

Conversation

3 participants
@tfennelly
Member

tfennelly commented May 1, 2016

@kohsuke Can you see any issue with adding this?

I need it in a plugin to cleanup some user info attached to the HttpSession.

tfennelly added some commits May 1, 2016

removed JenkinsHttpSessionListenerTest
Will readd if I can figure out why it doesn't work. Something to do with JenkinsRule and the listener config for the container (I'm guessing).
+ *
+ * @author <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>
+ */
+public final class JenkinsHttpSessionListener implements javax.servlet.http.HttpSessionListener {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 1, 2016

Member

NIT: I doubt Jenkins prefix is required

@oleg-nenashev

oleg-nenashev May 1, 2016

Member

NIT: I doubt Jenkins prefix is required

This comment has been minimized.

@daniel-beck

daniel-beck May 3, 2016

Member

@Restricted(NoExternalUse.class)

@daniel-beck

daniel-beck May 3, 2016

Member

@Restricted(NoExternalUse.class)

This comment has been minimized.

@tfennelly

tfennelly May 3, 2016

Member

+1 should be marked as @Restricted

@tfennelly

tfennelly May 3, 2016

Member

+1 should be marked as @Restricted

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 3, 2016

Member

Actually, why "http"? Will we have something else for Https?

@oleg-nenashev

oleg-nenashev May 3, 2016

Member

Actually, why "http"? Will we have something else for Https?

This comment has been minimized.

@daniel-beck

daniel-beck May 3, 2016

Member

@oleg-nenashev Could you explain what you're referring to? There's no such thing as javax.servlet.https

http://docs.oracle.com/javaee/6/api/javax/servlet/http/package-summary.html

@daniel-beck

daniel-beck May 3, 2016

Member

@oleg-nenashev Could you explain what you're referring to? There's no such thing as javax.servlet.https

http://docs.oracle.com/javaee/6/api/javax/servlet/http/package-summary.html

+ }
+ }
+
+ private static List<HttpSessionListener> getExtensionPoints() {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 1, 2016

Member

I think it should be the "all()" method in the extension point. At least we have it in other extension points

@oleg-nenashev

oleg-nenashev May 1, 2016

Member

I think it should be the "all()" method in the extension point. At least we have it in other extension points

This comment has been minimized.

@daniel-beck

daniel-beck May 3, 2016

Member

Should also return an ExtensionList. Why is this different from other implementations of the same mechanism?

@daniel-beck

daniel-beck May 3, 2016

Member

Should also return an ExtensionList. Why is this different from other implementations of the same mechanism?

This comment has been minimized.

@tfennelly

tfennelly May 3, 2016

Member

@oleg-nenashev "all" what? The enclosing type is not the same as the return type (JenkinsHttpSessionListener Vs HttpSessionListener, which is the extension point). The function is private, JenkinsHttpSessionListener is @Restricted(NoExternalUse.class) and is the only thing that should ever look for these extension points. Is this something you (or @daniel-beck) feel strongly about?

@daniel-beck why return ExtensionList i.e. a less general type? That seems bad to me but I'll do it if it's something you feel strongly about.

@tfennelly

tfennelly May 3, 2016

Member

@oleg-nenashev "all" what? The enclosing type is not the same as the return type (JenkinsHttpSessionListener Vs HttpSessionListener, which is the extension point). The function is private, JenkinsHttpSessionListener is @Restricted(NoExternalUse.class) and is the only thing that should ever look for these extension points. Is this something you (or @daniel-beck) feel strongly about?

@daniel-beck why return ExtensionList i.e. a less general type? That seems bad to me but I'll do it if it's something you feel strongly about.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 3, 2016

Member

The function is private, JenkinsHttpSessionListener is @restricted(NoExternalUse.class) and is the only thing that should ever look for these extension points.

I disagree with it. Jenkins is designed as an extensible system, so such presumptions do not work well for extension points.

@oleg-nenashev

oleg-nenashev May 3, 2016

Member

The function is private, JenkinsHttpSessionListener is @restricted(NoExternalUse.class) and is the only thing that should ever look for these extension points.

I disagree with it. Jenkins is designed as an extensible system, so such presumptions do not work well for extension points.

This comment has been minimized.

@daniel-beck

daniel-beck May 3, 2016

Member

@tfennelly Consistency mainly, something Jenkins already lacks.

Since you point out the different types (good point!), why not introduce all() in the other class and have it be used in the methods from this class?

@daniel-beck

daniel-beck May 3, 2016

Member

@tfennelly Consistency mainly, something Jenkins already lacks.

Since you point out the different types (good point!), why not introduce all() in the other class and have it be used in the methods from this class?

This comment has been minimized.

@tfennelly

tfennelly May 3, 2016

Member

@oleg-nenashev

Please look again. JenkinsHttpSessionListener is not the extension point - it's the web container hook and is not something that anyone should ever need to reference.

jenkins.util.HttpSessionListener is the extension point and it is not restricted in any way. Plugins can extend that and use it to get the HttpSession lifecycle events.

@tfennelly

tfennelly May 3, 2016

Member

@oleg-nenashev

Please look again. JenkinsHttpSessionListener is not the extension point - it's the web container hook and is not something that anyone should ever need to reference.

jenkins.util.HttpSessionListener is the extension point and it is not restricted in any way. Plugins can extend that and use it to get the HttpSession lifecycle events.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 1, 2016

Member

@tfennelly
Could you please clarify the use-cases of such ExtensionPoint (Better to do it in a JIRA issue). The implementation is more or less valid, but I cannot elaborate the interfaces

Member

oleg-nenashev commented May 1, 2016

@tfennelly
Could you please clarify the use-cases of such ExtensionPoint (Better to do it in a JIRA issue). The implementation is more or less valid, but I cannot elaborate the interfaces

@tfennelly

This comment has been minimized.

Show comment
Hide comment
@tfennelly

tfennelly May 1, 2016

Member

@oleg-nenashev

Hey Oleg. The set of use cases is the same as the use cases for HttpSessionListener itself.

HttpSessionListener implementations need to be defined in web.xml, which is obviously not possible for plugins.

My specific use case is that I'm building an Server Sent Events (SSE) gateway plugin. It needs to manage client event subscriptions and needs to be able to clean up at different times, including when the user session times out. This ExtensionPoint makes that possible. I have not seen anything else like it in Jenkins, but maybe I missed something.

Member

tfennelly commented May 1, 2016

@oleg-nenashev

Hey Oleg. The set of use cases is the same as the use cases for HttpSessionListener itself.

HttpSessionListener implementations need to be defined in web.xml, which is obviously not possible for plugins.

My specific use case is that I'm building an Server Sent Events (SSE) gateway plugin. It needs to manage client event subscriptions and needs to be able to clean up at different times, including when the user session times out. This ExtensionPoint makes that possible. I have not seen anything else like it in Jenkins, but maybe I missed something.

@tfennelly

This comment has been minimized.

Show comment
Hide comment
@tfennelly

tfennelly May 1, 2016

Member

Duh ... I missed the fact that they can be registered via the ServletContext. Or at least it looks like that's possible. Will close this for now.

Member

tfennelly commented May 1, 2016

Duh ... I missed the fact that they can be registered via the ServletContext. Or at least it looks like that's possible. Will close this for now.

@tfennelly tfennelly closed this May 1, 2016

@tfennelly tfennelly reopened this May 2, 2016

@tfennelly

This comment has been minimized.

Show comment
Hide comment
@tfennelly

tfennelly May 2, 2016

Member

Reopening since using @WebListener, ServletContainerInitializer etc do not work because plugin is not part of the main .war deployment (and so are not picked up by any scan) + are Servlet 3.0 + only (that might not really be a problem anyway - 3.0 may be a min requirement now I think?).

Member

tfennelly commented May 2, 2016

Reopening since using @WebListener, ServletContainerInitializer etc do not work because plugin is not part of the main .war deployment (and so are not picked up by any scan) + are Servlet 3.0 + only (that might not really be a problem anyway - 3.0 may be a min requirement now I think?).

+ * <p>
+ * Allows plugins to listen to {@link HttpSession} lifecycle events.
+ *
+ * @author <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>

This comment has been minimized.

@daniel-beck

daniel-beck May 3, 2016

Member

@since TODO

@daniel-beck

daniel-beck May 3, 2016

Member

@since TODO

This comment has been minimized.

@tfennelly

tfennelly May 3, 2016

Member

okidoki

@tfennelly

tfennelly May 3, 2016

Member

okidoki

This comment has been minimized.

@tfennelly

tfennelly May 3, 2016

Member

This is done

@tfennelly

tfennelly May 3, 2016

Member

This is done

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 3, 2016

Member

No tests.

Member

daniel-beck commented May 3, 2016

No tests.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 3, 2016

Member

In general, I like to see JENKINS issues for non-trivial changes.

Member

daniel-beck commented May 3, 2016

In general, I like to see JENKINS issues for non-trivial changes.

@tfennelly

This comment has been minimized.

Show comment
Hide comment
@tfennelly

tfennelly May 3, 2016

Member

@daniel-beck yeah I was trying to add a simple test for it (in 2b521a5) but couldn't get it to work. Will look again. I have tested it manually though and it works fine.

As for the use case ... I replied to the same question from @oleg-nenashev in #2303 (comment)

Member

tfennelly commented May 3, 2016

@daniel-beck yeah I was trying to add a simple test for it (in 2b521a5) but couldn't get it to work. Will look again. I have tested it manually though and it works fine.

As for the use case ... I replied to the same question from @oleg-nenashev in #2303 (comment)

@tfennelly

This comment has been minimized.

Show comment
Hide comment
@tfennelly

tfennelly May 3, 2016

Member

As for the specific use case ... see https://github.com/tfennelly/sse-gateway-plugin. It's a work in progress, but one thing it needs to be able to do is to subscribe and unsubscribe for events that are async pushed to the Jenkins UI.

This change provides a way for the plugin to listen for user session timeouts and so perform cleanups relating to that user session.

Member

tfennelly commented May 3, 2016

As for the specific use case ... see https://github.com/tfennelly/sse-gateway-plugin. It's a work in progress, but one thing it needs to be able to do is to subscribe and unsubscribe for events that are async pushed to the Jenkins UI.

This change provides a way for the plugin to listen for user session timeouts and so perform cleanups relating to that user session.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 3, 2016

Member

Marked as WiP till there are some tests and API/javadoc adjustments

Member

oleg-nenashev commented May 3, 2016

Marked as WiP till there are some tests and API/javadoc adjustments

@tfennelly tfennelly closed this May 3, 2016

@tfennelly tfennelly reopened this May 3, 2016

@tfennelly

This comment has been minimized.

Show comment
Hide comment
@tfennelly

tfennelly May 3, 2016

Member

@oleg-nenashev there's something about the Jetty instance in JenkinsRule that has session management turned off. I'm not able to get 2b521a5 working via JenkinsRule - HttpSessions are never created, even if I add a RootAction impl in the test that explicitly calls StaplerRequest.getSession(true).

I've tested it manually and it works fine. I really don't think it makes sense to spend a load more time on this.

Member

tfennelly commented May 3, 2016

@oleg-nenashev there's something about the Jetty instance in JenkinsRule that has session management turned off. I'm not able to get 2b521a5 working via JenkinsRule - HttpSessions are never created, even if I add a RootAction impl in the test that explicitly calls StaplerRequest.getSession(true).

I've tested it manually and it works fine. I really don't think it makes sense to spend a load more time on this.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 6, 2016

Member

What was the purpose of the test case revert commit?

Member

oleg-nenashev commented May 6, 2016

What was the purpose of the test case revert commit?

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 6, 2016

Member

@oleg-nenashev Tom's last comment and the commit message explain it, I think.

removed JenkinsHttpSessionListenerTest

Will readd if I can figure out why it doesn't work. Something to do with JenkinsRule and the listener config for the container (I'm guessing).

Member

daniel-beck commented May 6, 2016

@oleg-nenashev Tom's last comment and the commit message explain it, I think.

removed JenkinsHttpSessionListenerTest

Will readd if I can figure out why it doesn't work. Something to do with JenkinsRule and the listener config for the container (I'm guessing).

@tfennelly

This comment has been minimized.

Show comment
Hide comment
@tfennelly

tfennelly May 6, 2016

Member

@oleg-nenashev @daniel-beck I'm going to merge this unless I hear of a good reason not to.

Member

tfennelly commented May 6, 2016

@oleg-nenashev @daniel-beck I'm going to merge this unless I hear of a good reason not to.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 6, 2016

Member

I'm fine with the current implementation. But I'm not so happy about the statement above

Member

oleg-nenashev commented May 6, 2016

I'm fine with the current implementation. But I'm not so happy about the statement above

@tfennelly

This comment has been minimized.

Show comment
Hide comment
@tfennelly

tfennelly May 6, 2016

Member

@oleg-nenashev sorry Oleg, what statement are you not happy with?

Member

tfennelly commented May 6, 2016

@oleg-nenashev sorry Oleg, what statement are you not happy with?

@tfennelly tfennelly merged commit 78ad236 into jenkinsci:master May 8, 2016

1 check passed

Jenkins This pull request looks good
Details

@tfennelly tfennelly deleted the tfennelly:session-listener-ep branch May 8, 2016

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 8, 2016

Member

@tfennelly

Oleg, what statement are you not happy with?

  1. The commits have not been squashed
  2. "I'm going to merge this unless I hear of a good reason not to." does not sound a fine community interaction to me. It could be written in different manner
Member

oleg-nenashev commented May 8, 2016

@tfennelly

Oleg, what statement are you not happy with?

  1. The commits have not been squashed
  2. "I'm going to merge this unless I hear of a good reason not to." does not sound a fine community interaction to me. It could be written in different manner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment