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

ServletOutputStream.isReady() returns true while it is not yet ready #1755

Closed
cy6erGn0m opened this issue Aug 18, 2017 · 16 comments

Comments

@cy6erGn0m
Copy link

commented Aug 18, 2017

The general contract for first write is the following

  • set write listener via setWriteListener(...)
  • suspend and await for onWritePossible() invocation
  • check for isReady() and if it returned true then write(..)

However isReady() just after write listener has been set is true. Shouldn't it be false until first onWritePossible() callback invocation? Or it should be possible to invoke write if isRead() returned true in spite of that write listener will be scheduled later. I'd say the first option looks better

@sbordet

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2017

Method isReady() is not a method that should be called lightly. It's not a query method that returns a field.

It's a method that, when it returns false, has the side effect of telling the container to schedule a call to onWritePossible().

As such, calling isReady() from service() (or doGet(), etc.) is not a good idea.

Is it correct that it returns true, however, because you can write to the OutputStream if you want to, and the container will invoke onWritePossible() the first time without the need for isReady() to be invoked by the application (and returning false).

Yes, the API is weird.

@gregw

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2017

initially writing is always possible as the only thing that can prevent writing is if a prior write has not completed. so onWritePossible is scheduled as soon as a listener is set.

even if not possible, the code should always check isReady before writing so a spurious call to onWritePossible should not be a problem.

@gregw gregw closed this Aug 18, 2017

@cy6erGn0m

This comment has been minimized.

Copy link
Author

commented Aug 22, 2017

Just note that the following will crash in write(...)

out.setWriteListener(..)
if (out.isReady()) { out.write( ... ) }
@sbordet

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

"Crash" ? Do you have details ?

@cy6erGn0m

This comment has been minimized.

Copy link
Author

commented Aug 22, 2017

IllegalStateException. Let me try to find where exactly

@cy6erGn0m

This comment has been minimized.

Copy link
Author

commented Aug 22, 2017

java.lang.IllegalStateException: null
	at org.eclipse.jetty.server.HttpOutput.write(HttpOutput.java:455)
	.....

at attempt to switch state

if (!_state.compareAndSet(OutputState.PENDING, OutputState.ASYNC))
    throw new IllegalStateException();
@sbordet

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

I'm not sure that is legal usage of the Servlet API, but I'll defer to @gregw on that.

But what's the point of that code ?
If you set a WriteListener, why you want to try to write the next line ?
Just put the next line inside onWritePossible() and then container will call you.

@cy6erGn0m

This comment has been minimized.

Copy link
Author

commented Aug 22, 2017

Yeah, I know that the correct way is to wait for onWritePossible() but I was forced to do so to workaround tomcat's bug that sometimes doesn't invoke callback at all for upgraded output stream. So I finally ended up with a temporary additional check for ServletOutputStream class name :(

Actually I am facing issues with async Servlet API from time to time because the specification is too often not clear about threading and usage scenarios

@sbordet

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

Well, I guess we can avoid that exception, so let me reopen this issue.

@sbordet sbordet reopened this Aug 22, 2017

@gregw

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

The problem is that code like if (out.isReady()) { out.write( ... ) } is a race condition if you don't respect the servlet containers threading. If any thread can execute that code while a call to onWritePossible might occur, then you will have a race between the test and the write and one thread may fail.

So I'm not really inclined to try to "fix" this because I'm not sure how to fix it - specially if the setWriteListener is called from a non container thread and thus the onWritePossible call will be dispatched and in a race.

@sbordet

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

@gregw, however this may be legal:

protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
    AsyncContext context = request.startAsync();
    ServletOutputStream out = response.getOutputStream();
    out.setWriteListener(...);
    while (out.isReady()) {
        out.write(...);
    }
}

Above, we don't exit service() until we have written it all, or we're not ready.
In case of the latter, onWritePossible() will be called.

I recognize that there is code duplication, as you have to write the same code after setWriteListener(), and inside onWritePossible(), and so would be better to write it once in onWritePossible(). But still, seems legal code to me ?

@cy6erGn0m

This comment has been minimized.

Copy link
Author

commented Aug 22, 2017

Of course interference between "main" thread and one that will be running onWritePossible callback is potentially dangerous so it is the user's responsibility to avoid concurrent write loops. It is complicated, dangerous and error-prone but I believe it should be some clear position about this particular scenario: it should be either allowed or strictly prohibited.

@cy6erGn0m

This comment has been minimized.

Copy link
Author

commented Aug 22, 2017

I personally do such a synchronization so write loop is spinning while possible on caller thread (could be any thread, possibly container's processor thread) and then moving to onWritePossible()'s thread if isReady() return false but of course write loops are never running concurrently on different threads.

@gregw

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

@joakime

This comment has been minimized.

Copy link
Member

commented Jun 28, 2018

@gregw and @sbordet this issue needs an update (or should be closed)

@gregw

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

I'm closing this. It is intrinsically a race condition for which there is no absolute fix.

@gregw gregw closed this Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.