6035 memory leaks #6143
6035 memory leaks #6143
Conversation
|
Can one of the admins verify this patch? |
|
One specific area I want somebody who understands how different types of authentication work to review: It's possible that it's not strictly necessary in some of the places above (in some cases the user may be redirected back to the login page, for example, and the session timeout will be increased there - ?) - but it shouldn't hurt either. I'm more concerned about missing something. |
There was a problem hiding this comment.
"Request changes" seems too strong because this is a really good pull request but @landreev if you would document the new :LoginSessionTimeout option both in the Installation Guide (config page) and maybe at doc/release-notes/4.17-release-notes.md (new process) it would be much appreciated.
Also, what about log out? Should we consider destroying the session on logout? Here's a TODO comment in diff:
diff --git a/src/main/java/edu/harvard/iq/dataverse/DataverseHeaderFragment.java b/src/main/java/edu/harvard/iq/dataverse/DataverseHeaderFragment.java
index 0085c395f..0fedc38c6 100644
--- a/src/main/java/edu/harvard/iq/dataverse/DataverseHeaderFragment.java
+++ b/src/main/java/edu/harvard/iq/dataverse/DataverseHeaderFragment.java
@@ -220,6 +220,7 @@ public class DataverseHeaderFragment implements java.io.Serializable {
public String logout() {
dataverseSession.setUser(null);
dataverseSession.setStatusDismissed(false);
+ // TODO: destroy session here somehow?
String redirectPage = navigationWrapper.getPageFromContext();
try {
| HttpSession httpSession = (HttpSession) FacesContext.getCurrentInstance().getExternalContext().getSession(false); | ||
|
|
||
| if (httpSession != null) { | ||
| logger.info("jsession: "+httpSession.getId()+" setting the lifespan of the session to " + systemConfig.getLoginSessionTimeout() + " minutes"); |
There was a problem hiding this comment.
This should probably be reduced to logger.fine.
| @@ -55,6 +55,7 @@ public String init() { | |||
| if (confirmEmailData != null) { | |||
| user = confirmEmailData.getAuthenticatedUser(); | |||
| session.setUser(user); | |||
| session.configureSessionTimeout(); // TODO: is this needed here? (it can't hurt, but still) | |||
There was a problem hiding this comment.
Yes, this is needed here. This is a builtin user who just reset their password.
src/main/java/edu/harvard/iq/dataverse/settings/SettingsServiceBean.java
Show resolved
Hide resolved
| <session-timeout> | ||
| 1440 | ||
| 10 |
There was a problem hiding this comment.
Nice and short. 10 minutes. Perfect. Let's try it. 😄
|
|
||
| .. code-block:: bash | ||
|
|
||
| #!/bin/sh |
There was a problem hiding this comment.
We could refactor this into a downloadable script and also continue to inline it. Here's an example of where we do that (the "clear timer" script): http://guides.dataverse.org/en/4.16/admin/troubleshooting.html#deployment-fails-ejb-timer-service-not-available
…s bound to it on logout. (#6035)
|
Tested, this is ready to go, pending a release notes update. |
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
closes #6035
Pull Request Checklist