Skip to content

Add null servlet context check#1414

Merged
tylerbenson merged 2 commits into
masterfrom
tyler/null-context
May 6, 2020
Merged

Add null servlet context check#1414
tylerbenson merged 2 commits into
masterfrom
tyler/null-context

Conversation

@tylerbenson
Copy link
Copy Markdown
Contributor

java.lang.NullPointerException
	at datadog.trace.instrumentation.servlet3.Servlet3Decorator.onContext(Servlet3Decorator.java:81)
	at datadog.trace.instrumentation.servlet3.Servlet3Decorator.onRequest(Servlet3Decorator.java:68)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:782)
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:755)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:547)
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:190)
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:485)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127)
	at org.eclipse.jetty.server.Server.handle(Server.java:500)
	at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:383)
	at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:547)
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:375)
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:270)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:103)
	at org.eclipse.jetty.io.ChannelEndPoint$2.run(ChannelEndPoint.java:117)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:806)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:938)
	at java.base/java.lang.Thread.run(Thread.java:844)

@tylerbenson tylerbenson requested a review from a team as a code owner April 30, 2020 14:33
Copy link
Copy Markdown
Contributor

@dougqh dougqh Apr 30, 2020

Choose a reason for hiding this comment

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

I think the comment is good. I'm wondering if we should add a test as well. Even if it is a bit artificial.

I think we also need to keep expanding our server smoke tests, but that's separate from this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added a test that fails without that change.

@tylerbenson tylerbenson force-pushed the tyler/null-context branch from acba74b to 2aad958 Compare May 5, 2020 20:44
@Override
Server startServer(int port) {
Server server = new Server(port)
ServletHandler handler = new ServletHandler()
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Key difference is ServletHandler vs ServletContextHandler used in the main set of tests.

@tylerbenson tylerbenson merged commit 819a8d0 into master May 6, 2020
@tylerbenson tylerbenson deleted the tyler/null-context branch May 6, 2020 14:01
@github-actions github-actions Bot added this to the 0.51.0 milestone May 6, 2020
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.

3 participants