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

#268 changes to authenticate when tomcat launched as service #301

Closed
wants to merge 1 commit into from

Conversation

vimil
Copy link
Contributor

@vimil vimil commented Dec 7, 2015

This a PR for issue 'Cannot log in automatically on machine where Tomcat service is running #268'

After reading the comments on the issue, I understand that current code is calling AcquireCredentialsHandle every-time instead of just once and reusing the handle in subsequent calls.

@dblock
Copy link
Collaborator

dblock commented Dec 7, 2015

This looks OK to me, maybe other people want to check it too. This definitely needs a CHANGELOG entry, please. Also something off with default tabs/spaces if you don't mind fixing.

@vimil
Copy link
Contributor Author

vimil commented Dec 8, 2015

Thanks for taking a look, I've fixed the tab space issue, Where is the change-log file located so I can update it?

@dblock
Copy link
Collaborator

dblock commented Dec 8, 2015

See https://github.com/dblock/waffle/blob/master/CHANGELOG.md. Also please squash the multiple commits. Thanks!

@vimil
Copy link
Contributor Author

vimil commented Dec 8, 2015

updated changelog and squashed commits

@dblock
Copy link
Collaborator

dblock commented Dec 8, 2015

LGTM, @hazendaz take a look and merge at will, I am still puzzled to why this makes a difference, but at the very least it's a performance optimization

@hazendaz
Copy link
Member

hazendaz commented Dec 8, 2015

@dblock I'll take a look at this in a day or so. I'll try to setup the scenerio as well to check it out.

@vimil
Copy link
Contributor Author

vimil commented Dec 8, 2015

Below is the link I found when searching for sample code to authenticate using kerebros on windows

http://www.informit.com/articles/article.aspx?p=20989&seqNum=3

@hazendaz
Copy link
Member

@vimil code has been merged into master. I had to run a build and did some trivial cleanup per eclipse warnings and opportunately to use constructor. I didn't get a chance to verify this myself but read all the background and looked over this. I think this is correct. My only concern was the comment on stackoverflow of possible handle leak. I've got concerns in that area in general and now that we are java 7, we could implement closeable to help out. I digress...anyway thank you for the contribution and feel free to contribute more. Some of these issues are extremely tricky to solve.

@hazendaz hazendaz closed this Dec 12, 2015
@hazendaz
Copy link
Member

this was merged on 405943b

@vimil
Copy link
Contributor Author

vimil commented Dec 12, 2015

Thanks @hazendaz for merging the pull request, the changes look good to me.

A different topic but I was wondering if we could remove guava dependency and create our on map implementation that expires keys after a time interval. This will make the waffle library lightweight.

@hazendaz
Copy link
Member

@vimil We use guava pretty extensively throughout waffle so it's not something to pull out based on this one usecase. Many other third parties use guava too so I don't think that makes waffle not lightweight. Every project I've used waffle in already had guava present. When we jump to java 8 only, we can revisit as 50% or more of guava is in java 8 now. And probably for our use case means our needs of guava would then go away. I would expect that sort of jump on waffle 2.0.x. I'll open an issue as a marker on this.

@vimil
Copy link
Contributor Author

vimil commented Dec 14, 2015

That makes sense @Hazendas. Thanks for the update

@jabbera
Copy link

jabbera commented Feb 10, 2016

Hello,

Any idea when 1.8.1 will be released. This particular is causing me a headache.

Thanks!
Mike

@hazendaz
Copy link
Member

I'll try to push a release shortly.

Sent from Outlook Mobile

On Wed, Feb 10, 2016 at 1:23 PM -0800, "Mike" notifications@github.com wrote:

Hello,

Any idea when 1.8.1 will be released. This particular is causing me a headache.

Thanks!
Mike


Reply to this email directly or view it on GitHub:
#301 (comment)

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