tyrus
  1. tyrus
  2. TYRUS-201

Wrong ServletInputStream#isReady() usage in TyrusHttpUpgradeHandler

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3
    • Component/s: None
    • Labels:
      None

      Description

              } finally {
                  if (is.isReady()) {
                      LOGGER.log(Level.SEVERE, "This shouldn't happen. ServletInputStream.isReady() returned true after reading all available() data.");
                  } else {
                      // everything is ok, all data consumed, waiting for next onDataAvailable call from web-core
                  }
              }
      

      It is not an error that isReady() returns true. There is a window where data can be available after the call available() and before isReady() is called.

      While isReady() returns true, you need to loop and read the data. Otherwise, the container won't call onDataAvailable() again. See the javadoc of ServerInputStream

        Activity

        Hide
        Pavel Bucek added a comment -

        javadoc did not match the implementation in this case (when I was originally implementing it).

        Problem here was that isReady() does some "black magic" and something went wrong when isReady was NOT called. And if I remember correctly, current implementation don't set "isReady" flag to true unless all data are processed.

        Is this coming from some testcase/possible bug or are you just reviewing the code?

        Show
        Pavel Bucek added a comment - javadoc did not match the implementation in this case (when I was originally implementing it). Problem here was that isReady() does some "black magic" and something went wrong when isReady was NOT called. And if I remember correctly, current implementation don't set "isReady" flag to true unless all data are processed. Is this coming from some testcase/possible bug or are you just reviewing the code?
        Hide
        jitu added a comment -

        I believe the isReady() behaviour changed after PRD/PR. Just came across this code which seems wrong.
        If the servlet container impl doesn't follow the javadoc, transfer this to GF. Let me add ShingWai to the watchers.

        Show
        jitu added a comment - I believe the isReady() behaviour changed after PRD/PR. Just came across this code which seems wrong. If the servlet container impl doesn't follow the javadoc, transfer this to GF. Let me add ShingWai to the watchers.
        Hide
        Pavel Bucek added a comment -

        OK, just.. I'm little hesitant to make changes here, it seems to work like we need. I don't see any real issue reported. Not saying I'm absolutely against that, just recalling troubles during figuring this out..

        Show
        Pavel Bucek added a comment - OK, just.. I'm little hesitant to make changes here, it seems to work like we need. I don't see any real issue reported. Not saying I'm absolutely against that, just recalling troubles during figuring this out..
        Hide
        Pavel Bucek added a comment -

        merged to the master branch (rev 8db2288cc86178b4ea1920c0e2062f8ebbfb8ef0)

        Show
        Pavel Bucek added a comment - merged to the master branch (rev 8db2288cc86178b4ea1920c0e2062f8ebbfb8ef0)

          People

          • Assignee:
            Pavel Bucek
            Reporter:
            jitu
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: