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

Fix #411 No Object Wrapper Identity requirement for dispatch #413

Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jul 29, 2021

Fix #411 No Object Wrapper Identity requirement for dispatch

Fix jakartaee#411 No Object Wrapper Identity requirement for dispatch
@gregw gregw marked this pull request as draft July 29, 2021 03:55
@gregw
Copy link
Contributor Author

gregw commented Oct 5, 2021

@markt-asf @stuartwdouglas what are your thoughts on this one? Any chance of it getting in 6.0? If not, then I'm interested to know if there would ever be a time we could do such changes?

I think it is probably our only opportunity to fix this issue - which makes servlets really bad for async applications at the benefit of vanishingly few applications. Note that I'm proposing keeping object identity in the filter chain, which is probably where most of the downcasting apps do their stuff - it is only when going via a dispatch that changes the paths etc. that I propose allowing identity to be broken in return for immutable references that work with async.

Copy link
Contributor

@stuartwdouglas stuartwdouglas left a comment

Choose a reason for hiding this comment

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

I think this looks good.

If this requirement was removed from the filter chain I would be a bit more conservative, but just removing it from RequestDispatcher and async dispatch is fine, and could greatly simplify our implementation.

@markt-asf
Copy link
Contributor

+1

@gregw gregw marked this pull request as ready for review October 7, 2021 01:07
@gregw
Copy link
Contributor Author

gregw commented Oct 7, 2021

OK then I think this is ready to merge. But I'll leave for a few days just in case others wish to comment.

@gregw gregw merged commit 92ae415 into jakartaee:master Oct 8, 2021
@gregw gregw deleted the issue-411-No-Dispatcher-ObjectWrapperIdentity branch October 8, 2021 23:24
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.

No object wrapper identity on dispatch
3 participants