{"action":"created","issue":{"url":"https://api.github.com/repos/eclipse/jetty.project/issues/247","repository_url":"https://api.github.com/repos/eclipse/jetty.project","labels_url":"https://api.github.com/repos/eclipse/jetty.project/issues/247/labels{/name}","comments_url":"https://api.github.com/repos/eclipse/jetty.project/issues/247/comments","events_url":"https://api.github.com/repos/eclipse/jetty.project/issues/247/events","html_url":"https://github.com/eclipse/jetty.project/issues/247","id":134128469,"number":247,"title":"Commit exception should provide more details","user":{"login":"jmcc0nn3ll","id":273327,"avatar_url":"https://avatars.githubusercontent.com/u/273327?v=3","gravatar_id":"","url":"https://api.github.com/users/jmcc0nn3ll","html_url":"https://github.com/jmcc0nn3ll","followers_url":"https://api.github.com/users/jmcc0nn3ll/followers","following_url":"https://api.github.com/users/jmcc0nn3ll/following{/other_user}","gists_url":"https://api.github.com/users/jmcc0nn3ll/gists{/gist_id}","starred_url":"https://api.github.com/users/jmcc0nn3ll/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jmcc0nn3ll/subscriptions","organizations_url":"https://api.github.com/users/jmcc0nn3ll/orgs","repos_url":"https://api.github.com/users/jmcc0nn3ll/repos","events_url":"https://api.github.com/users/jmcc0nn3ll/events{/privacy}","received_events_url":"https://api.github.com/users/jmcc0nn3ll/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/eclipse/jetty.project/labels/Bug","name":"Bug","color":"eb6420"}],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-02-16T23:40:48Z","updated_at":"2016-03-02T21:00:32Z","closed_at":"2016-03-02T16:08:56Z","body":"migrated from Bugzilla [#481131](https://bugs.eclipse.org/bugs/show_bug.cgi?id=481131)\nstatus REOPENED severity *trivial* in component *server* for *9.3.x*\nReported in version *unspecified* on platform *All*\nAssigned to: Project Inbox\n\nOn 2015-10-30 12:10:05 -0400, Gray wrote:\n\n> When a request has already been committed (typically by someone calling response.getWriter()), and then someone tries to do a redirect, an exception is thrown in Response.\n> \n>     public void resetBuffer()\n>     {\n>         if (isCommitted())\n>             throw new IllegalStateException(\"Committed\");\n>         _connection.getGenerator().resetBuffer();\n>     }\n> \n> This exception message is close to worthless.  Please consider changing to to be something like:\n> \n>     Response has already been committed.  Are you trying to change the status code after sending content?\n> \n> Or some such.  Every exception is an opportunity to provide feedback to the developer so one word exceptions should be expanded to provide more information.\n> \n> Thanks much.\n\nOn 2015-11-01 18:20:23 -0500, Greg Wilkins wrote:\n\n> Gray,\n> \n> sorry but the basic policy of Jetty is to avoid chatty long exception messages that are localized to a specific language.   Instead we try to provide all the relevant information in a minimal format that is not too locale specific.\n> \n> I don't agree that the current exception is worthless.   It is telling you that the response object is in an illegal start for a call to resetBuffer.  Moreover it says that the state is committed.\n> \n> Granted it does not say how the response got into a committed state, but there are many ways that can happen and sending content is only one of those (and does not necessarily commit the response either).   Changing status code is only one of many actions that are prohibited once the response is committed, so also can not be called out in the error message.\n\nOn 2015-11-01 22:12:16 -0500, Gray wrote:\n\n> Then we disagree on all fronts.  Exceptions are supposed to be helpful to a developer unless you excepted us to jump to the source every time.  A one word exception that is easy to generate that uses a word that means many things is a big fail.  Every time someone generates an exception using one of my libraries I evaluate whether more information could have helped the developer figure out the problem on their own.  The idea that we don't want to be \"chatty\" or that we are worried about non-localized messages is just misdirection.  If they don't know english then one word or 30 isn't going to be an issue.  And I guess being chatty takes up too many bytes or fills log files?  But I suspect I'm wasting my breath.  Best of luck.\n\nOn 2015-11-01 22:35:54 -0500, Greg Wilkins wrote:\n\n> Gray,\n> \n> you're not wasting your breath as I'm happy to discuss - and plenty of times I have been convinced to change my mind.\n> \n> However, in this case I just don't understand what information is missing from the exception?\n> \n> Your suggested text comes in two parts: \"Response has already been committed.\" and \"Are you trying to change the status code after sending content?\"\n> \n> I maintain that the current implementation conveys all the information in the first part of your suggestion:  The exception is thrown from the response, it is already in an illegal state for the request operation and that state is \"committed\".\n> \n> The second part is just guessing at a potential cause that will often be wrong and is also suggesting that you can't change status after writing content - which with the servlet spec you can (now why that is allowed is an entirely different discussion and out of both of out hands).\n> \n> I'm not expecting the developer to look at the code, as in this case it provides no additional information.  The test that fails is Response.isCommitted, which tells you no more than IllegalStateException(\"Committed\")\n> \n> So assuming we just disagree on the chatty aspects and localization aspects, what exactly is the extra information that you want to see in that exception?\n\nOn 2015-11-01 23:07:32 -0500, Gray wrote:\n\n> Thanks for asking for more info greg.\n> \n> The word \"committed\" is not necessarily going to be know by the developer.  It's a Jetty term.  \"Response has already been committed.\" is better than just one word because then the developer knows that something is wrong with the response as opposed to them maybe having a committed an exception or something?\n> \n> I'm the author of ORMLite a ORM library and I try to add a bit more information to an exception to give the developer more information.  Certainly I never have a one word exception.  For example, when I do reflection on object constructors that fails, I say \"maybe not public or a non-static inner class?\".  I put the question-mark to let them know that it is a guess but it typically covers the standard use cases (i.e. the 60% rule) and even if it is a miss, it gives the developer a bit more information for them to look into the problem.  I'm continually watching SO and my mailing list for people posting exceptions and I try to see the exception from their point of view.  How can it help them better.\n> \n> In terms of committed responses, as I read the code, all you have to do is to call getOutputStream() or getWriter() to switch state on the response.  The typical times I've seen this exception is when someone tries to return downloaded information _and_ then do a redirect.\n> \n> Thanks for listening.\n\nOn 2015-11-01 23:34:40 -0500, Greg Wilkins wrote:\n\n> (In reply to Gray from comment # 4)\n> > Thanks for asking for more info greg.\n> > \n> > The word \"committed\" is not necessarily going to be know by the developer. \n> > It's a Jetty term.  \"Response has already been committed.\" is better than\n> > just one word because then the developer knows that something is wrong with\n> > the response as opposed to them maybe having a committed an exception or\n> > something?\n> \n> Well committed state is actually a servlet specification state and the isCommitted method is part of the standard API.   I don't think it is unreasonable to expect a servlet developer to be familiar with the concept of committing a response.\n> \n> Perhaps I'm guilty of reductio ad absurdum, but to me \"has already been\" is a  verbose way of saying \"is\",  So IllegalStateException(\"Response is Committed\") is perhaps a better way of saying it, but then what other object other than the Response could I be throwing an ISE about?  So if I don't need to specify the object, then I'm just left with saying which state I'm in that is illegal.\n> \n> Maybe IllegalStateException(\"state=Committed\"), but that starts implying that there is actually a state variable called state... which there isn't\n> \n> \n> > In terms of committed responses, as I read the code, all you have to do is\n> > to call getOutputStream() or getWriter() to switch state on the response. \n> > The typical times I've seen this exception is when someone tries to return\n> > downloaded information _and_ then do a redirect.\n> \n> They are many ways of committing a response, but they do not include getOutputStream() or getWriter().   responses can be committed by an explicit flushBuffers(), a sendError(), writing more content than the buffer size, writing exactly the content length set as a header (or setContentLength), or an exception thrown, doing an include in some situations.  Plus the commit action could have happened in a filter or a previous dispatch during an async cycle or perhaps even during a forward/include RequestDispatch \n> \n> The resetBuffer may be called directly or as part of another call (to perhaps sendError), but it is not called to set the the status.\n> \n> There issue may occur because a response was committed when it should not have been OR because a resetBuffers (or related operation) was attempted when it should not have.\n> \n> Put together, there are many combinations of why this exception may occur and I suggesting just one is likely to misdirect as more developers as it potentially helps.\n> \n> If you can't say anything useful, then don't say anything at all.\n> \n> Anyway,  I'll reopen for a while.  If any of the other developers think I'm being too terse, then I'll let them come up with better text.\n\nOn 2015-11-02 00:35:41 -0500, Gray wrote:\n\n> All good points.  How about:\n> \n> \"Response already committed.  Status code and headers written.\"\n\nOn 2015-11-02 09:56:46 -0500, Gray wrote:\n\n> Just some followup.\n> \n> First off, you said that a servlet programmer should know the word committed but I bet fewer than 30% do.  I know at my company, none of our folks except me knew what \"IllegalStateException: Committed\" even meant.  You have to remember that many are dealing with servlets at the lever of public ModelView handler(String someParam) { ... }.\n> \n> It maybe easy to figure out what the exception means when you call response.sendError(...) and get that exception but when you are returning new ModelView(\"...\") from a handler method and get that exception, it is far less obvious.\n\n"},"comment":{"url":"https://api.github.com/repos/eclipse/jetty.project/issues/comments/191427972","html_url":"https://github.com/eclipse/jetty.project/issues/247#issuecomment-191427972","issue_url":"https://api.github.com/repos/eclipse/jetty.project/issues/247","id":191427972,"user":{"login":"j256","id":1749962,"avatar_url":"https://avatars.githubusercontent.com/u/1749962?v=3","gravatar_id":"","url":"https://api.github.com/users/j256","html_url":"https://github.com/j256","followers_url":"https://api.github.com/users/j256/followers","following_url":"https://api.github.com/users/j256/following{/other_user}","gists_url":"https://api.github.com/users/j256/gists{/gist_id}","starred_url":"https://api.github.com/users/j256/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/j256/subscriptions","organizations_url":"https://api.github.com/users/j256/orgs","repos_url":"https://api.github.com/users/j256/repos","events_url":"https://api.github.com/users/j256/events{/privacy}","received_events_url":"https://api.github.com/users/j256/received_events","type":"User","site_admin":false},"created_at":"2016-03-02T21:00:32Z","updated_at":"2016-03-02T21:00:32Z","body":"Looks good.  Thanks much."}}