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

Fix race condition between shutdown and parsing in Http1ServerStage (#1487) #1675

Merged

Conversation

@bryce-anderson
Copy link
Member

@bryce-anderson bryce-anderson commented Feb 24, 2018

Fixes #1487.

The Http1ServerStage doesn't consider whether the stage has been
shutdown from within it's request loop. When the stage is shutdown, it
is reset, which can race with a read-and-parse operation happening in
another thread.

To fix it, we restrict concurrent access to the parser and flag whether
the stage has been shutdown, halting parsing of the prelude if
disconnect has happened.

@bryce-anderson
Copy link
Member Author

@bryce-anderson bryce-anderson commented Feb 24, 2018

There are more problems in there, specifically that if you hold onto a request body and attempt to use it after a dispatch has ended, you'll get some spooky things happening. However, that is a different, and more more involved fix, so I didn't include it here.

The Http1ServerStage doesn't consider whether the stage has been
shutdown from within it's request loop. When the stage is shutdown, it
is reset, which can race with a read-and-parse operation happening in
another thread.

To fix it, we restrict concurrent access to the parser and flag whether
the stage has been shutdown, halting parsing of the prelude if
disconnect has happened.
@bryce-anderson bryce-anderson force-pushed the issue/1487_requestLoopNPE branch from 786880a to 9b2307c Feb 24, 2018
Copy link
Member

@rossabaker rossabaker left a comment

Is there any reason to fret about a performance hit on this with all the new synchronization? I imagine contention is generally low and it's probably insignificant in the grand scheme.

Here's where I wish we had automated benchmarks.

@bryce-anderson
Copy link
Member Author

@bryce-anderson bryce-anderson commented Feb 25, 2018

Contention should be minimal (and if it happens, thats when we really want the lock 😄) so I'd expect perf to be effectively unchanged in the grand scheme of things. That said, I don't have any data either way, that's just my intuition.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Feb 25, 2018

That's my intuition, too. Let's do it.

@rossabaker rossabaker merged commit c52f974 into http4s:release-0.18.x Feb 25, 2018
2 checks passed
SimY4
Copy link

SimY4 commented on 9b2307c Feb 28, 2018

Sorry, it's probably not my place to give comments. I was just browsing through the list of changes.

But don't you think that this field should be marked as @volatile? Because synchronization will not protect you from stale reads

bryce-anderson
Copy link
Member

bryce-anderson commented on 9b2307c Feb 28, 2018

It's only accessed from with a synchronized block, so I think we're good.

SimY4
Copy link

SimY4 commented on 9b2307c Feb 28, 2018

Synchronization will guarantee that all writes will happen in order. It will not guarantee that latest written value will be read. Without marking this field as volatile thread can easily cache variable's value and not even bother checking if value was updated. That what volatile will protect you against

bryce-anderson
Copy link
Member

bryce-anderson commented on 9b2307c Mar 1, 2018

What you're describing is inconsistent with the JVM memory model, which can be found here.

SimY4
Copy link

SimY4 commented on 9b2307c Mar 1, 2018

It's not, have a look at this piece: JLS 17.4.5 Happens before

If the reordering produces results consistent with a legal execution, it is not illegal. If two actions share a happens-before relationship, they do not necessarily have to appear to have happened in that order to any code with which they do not share a happens-before relationship. Writes in one thread that are in a data race with reads in another thread may, for example, appear to occur out of order to those reads.

From the same chapter:

A write to a volatile field (§8.3.1.4) happens-before every subsequent read of that field.

bryce-anderson
Copy link
Member

bryce-anderson commented on 9b2307c Mar 1, 2018

  • 17.4.5. Happens-before Order: If an action x synchronizes-with a following action y, then we also have hb(x, y).
  • 17.4.4. Synchronization Order: An unlock action on monitor m synchronizes-with all subsequent lock actions on m (where "subsequent" is defined according to the synchronization order).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants