search

logzio / guice-jersey

Guice module for starting Jetty based rest server with Jersey
Apache License 2.0
40 stars 11 forks source link

Allow passing JettyConfiguration #7

Closed adavidai closed 6 years ago

adavidai commented 6 years ago

At this point controlling the Jetty thread pool configuration. As Jetty's default thread pool is unblocked, it might cause an overloaded server to crash...

Recommended settings are the jetty original defaults (minThreads=8, maxThreads=200, idleTimeout=60000), and maxQueueSize=maxThreads=200 (to avoid queued requests being processed even thought the client might have timed out already)

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-24.0%) to 69.325% when pulling 66463d62efc8a0b0a4494b07115be0d92bb2ce2d on jetty-server-with-limited-requests-queue-size-support into b35836b6ad268b680519215d0c20916d8d05aaf6 on master.

asafm commented 6 years ago

@adavidai - I would add a quick test, firing up Jetty with 2 threads max and see the request 3 is rejected

adavidai commented 6 years ago

@asafm, I disagree with you here -

  1. We are not testing Jetty \ Jersey code (testing each project should be taken care of in\by the project itself).
  2. Async tests are not reliable or steady enough - in my opinion their cons outweigh their pros

In a similar way, I do not want to add a test that the process does run out of memory \ stops returning results to jersey calls when the queue size is not set.

adavidai commented 6 years ago

@asafalima, @asafm , the test turns out to be more complicated than expected due to the fact that Jetty uses the same thread pool for multiple purposes (namely acceptors and selectors). That forces setting the threads num to a higher number, and means you do not control how many of those threads are actually available to do the work you expect them to do.

It will require more research, hope to get to it in the future.

I pushed the fixes you requested, as well as the current version of the test which is failing in case someone else wants to pick this up or has any insights to contribute (the test passes when ran separately but fails as part of the test suite on "WARN o.e.j.u.t.QueuedThreadPool: org.eclipse.jetty.util.thread.QueuedThreadPool@qtp505231702{STARTED,6<=6<=6,i=5,q=0} rejected acceptor-0@1bb1fde8", which yields a RejectedExecutionException on that acceptor).

segalziv commented 6 years ago

too old, need revisit, closing for now. please re-open if/when relevant