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.

Sign up for GitHub

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

Jump to bottom

avoid constructing objects if not needed #248

Merged
merged 1 commit into from
Jun 10, 2014
Merged

avoid constructing objects if not needed #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion project-code/core/common/src/main/scala/play/core/server/servlet/Play2Server.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ object Play2WarServer {
def stop(sc: ServletContext) = {
synchronized {
if (started.getAndSet(false)) {
Copy link

Choose a reason for hiding this comment

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

There's a bug here, started is true before playService is non-None.
Also, there's a bug in def apply where it will overwrite the current playServer without stopping it.

Copy link

Choose a reason for hiding this comment

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

I'd recommend to get rid of started since if playServer.isDefined it is started

Copy link
Member Author

Choose a reason for hiding this comment

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

thx for pointing this out. I'll check that.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is actually very strange. I do not understand the synchronized block around an AtomicBoolean.
I'll need some time and investigation to check why this code exists.

Copy link
Member

Choose a reason for hiding this comment

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

Probably not needed anymore.

playServer.map(_.stop())
playServer.foreach(_.stop())
sc.log("Play server stopped")
}
}
Expand Down
15 changes: 7 additions & 8 deletions project-code/core/common/src/main/scala/play/core/server/servlet/RequestHandler.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,18 +93,17 @@ trait HttpServletRequestHandler extends RequestHandler {

protected def setHeaders(headers: Map[String, String], httpResponse: HttpServletResponse): Unit = {
// Set response headers
headers.filterNot(_ ==(CONTENT_LENGTH, "-1")).foreach {
headers.foreach {
case (CONTENT_LENGTH, "-1") => // why is it skip?

// Fix a bug for Set-Cookie header.
// Multiple cookies could be merged in a single header
// but it's not properly supported by some browsers
case (name@play.api.http.HeaderNames.SET_COOKIE, value) => {
getServletCookies(value).map {
c => httpResponse.addCookie(c)
}
}
case (name, value) if name.equalsIgnoreCase(play.api.http.HeaderNames.SET_COOKIE) =>
getServletCookies(value).foreach(httpResponse.addCookie)

case (name, value) => httpResponse.setHeader(name, value)
case (name, value) =>
httpResponse.setHeader(name, value)
}
}

Expand Down Expand Up @@ -230,7 +229,7 @@ trait HttpServletRequestHandler extends RequestHandler {

} // end match foreach

}.map { _ => cleanup() }
}.onComplete { _ => cleanup() }
}
}

Expand Down
12 changes: 5 additions & 7 deletions project-code/core/servlet31/src/main/scala/play/core/server/servlet31/RequestHandler31.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,9 @@ class Play2Servlet31RequestHandler(servletRequest: HttpServletRequest)

if (servletInputStream.isFinished) {
Logger("play.war.servlet31").trace("will extract result from nextStep")
nextStep.run.map { a =>
result.completeWith(nextStep.run.andThen { case _ =>
Logger("play.war.servlet31").trace("extract result from nextStep")
result.success(a)
}(exContext)
}(exContext))
}
nextStep
}(exContext)
Expand All @@ -123,10 +122,9 @@ class Play2Servlet31RequestHandler(servletRequest: HttpServletRequest)
if (!onDataAvailableCalled) {
// some containers, like Jetty, call directly onAllDataRead without calling onDataAvailable
// when no data should be consumed
iteratee.run.map { a =>
result.completeWith(iteratee.run.andThen { case _ =>
Logger("play.war.servlet31").trace("onAllDataRead: extract result from iteratee")
result.success(a)
}(exContext)
}(exContext))
}
}

Expand Down Expand Up @@ -239,7 +237,7 @@ class Play2Servlet31RequestHandler(servletRequest: HttpServletRequest)
}

override protected def pushPlayResultToServletOS(futureResult: Future[SimpleResult], cleanup: () => Unit): Unit = {
getHttpResponse().getHttpServletResponse map { httpResponse =>
getHttpResponse().getHttpServletResponse.foreach { httpResponse =>

val out = httpResponse.getOutputStream.asInstanceOf[ServletOutputStream]

Expand Down