Out of memory issue for websocket client

python-tornado bionic debdiff

python tornado xenial

[sts-sponsor][Bionic]

Is this your final debdiff ?

* The bionic debdiff is incomplete as it only contains d/changelog.
* Version in d/changelog should be "4.5.3-1ubuntu0.2" instead of "4.5.3-1ubuntu0.1.1" since the previous version was "4.5.3-1ubuntu0.1"

[sts-sponsor][xenial]

d/changelog mentioned 3 patches:
  * d/p/0001-read-queue-of-1-message.patch
  * d/p/0001-Remove-unused-import.patch
  * d/p/0001-test-Skip-test_source_port_fail-when-running-as-root.patch

but d/p/series only has 2 of them ?
+0001-read-queue-of-1-message.patch
+0001-Remove-unused-import.patch

Having all your patches starting by 0001 can be confusing for future reference. I would suggest since it's a patchset fixing the same bug to order them and use the bug # for making sure folks know it was a patchset to fix this bug, as follows:

0001-lp1903733-read-queue-of-1-message.patch
0002-lp1903733-Remove-unused-import.patch
and so on ....

attached screenshot of terminal setup and running

updated bionic python-tornado package

[sts-sponsors][Review]

Bionic debdiff nitpicks:

* Change 4.5.3-1ubuntu0.1.1 -> 4.5.3-1ubuntu0.2
* Change (LP#:1903733) to (LP: #1903733)

Xenial debdiff nitpicks:
* Change (LP#:1903733) to (LP: #1903733)

[sts-sponsors][Review]

Quilt header for X and B:

* Add the commit link, not the PR please.
* Clean-up the launchpad.net bug -> https://launchpad.net/bugs/1903733

+Origin: upstream, https://github.com/tornadoweb/tornado/pull/2351/commits/20becca336caae61cd24f7afba0e177c0a210c70
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/python-tornado/+bug/1903733

ah ok i see, I will fix today. Thanks!

v2 of python-tornado bionic lp#1903733

v2 of xenial python-tornado package upload

[sts-sponsors][xenial]

Sponsored.

Thanks for your contribution Heather !

- Eric

[sts-sponsors][bionic]

Sponsored.

Thanks for your contribution Heather !

- Eric

Ok, the patches *seem* fine (especially that they're cherry-picks), but this SRU seems to be missing the regression potential analysis (per the [Where problems could occur] section). I don't know the code enough to feel if and which parts might regress after we land this change. Could you update the bug description with such information? Thanks!

Hello Heather, or anyone else affected,

Accepted python-tornado into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/python-tornado/4.5.3-1ubuntu0.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Heather, or anyone else affected,

Accepted python-tornado into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/python-tornado/4.2.1-1ubuntu3.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

All autopkgtests for the newly accepted python-tornado (4.5.3-1ubuntu0.2) for bionic have finished running.
The following regressions have been reported in tests triggered by the package:

ipykernel/4.8.2-2 (i386)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/bionic/update_excuses.html#python-tornado

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Link to autopackage source test https://launchpad.net/ubuntu/+source/python-tornado/4.5.3-1ubuntu0.2

Kicked the autopackage tests again.. passed 2nd time around

I don't believe what they have done with implementing a Queue(1) solves the client oom errors.
This actually didn't solve the problem because the underlying implementation still uses collection.deque() which was the issue to begin with.

websocket.py
self.read_queue = Queue(1)

queues.py
class Queue(object):
        self._getters = collections.deque([]) # Futures.
        self._putters = collections.deque([]) # Pairs of (item, Future).

I went through the code with @hypothetical-lemon and the patches do appear to fix the issue; with the previous code they were adding new messages into a deque without any throttling, but now they are using a different queue class that, while it still uses deques internally, returns a future object for any queued message. The throttling is then implemented in the websocket's read queue, which delays reading more bytes from the actual low-level socket until after the future object completes. This should restrict the queue in the tornado websocket class to no more than 1 queued message.

Additionally, as this has always been a completely theoretical "problem" (even the upstream bug was entirely theoretical, with nobody involved reproducing the problem), we have no clear and easy reproducer. However, all the package's test cases passed, indicating no regression due to the patches.

Thus, marking this as verified.

Thank you for digging into the verification, Heather, Dan. Seeing the change and reading the verification comments, I agree with the rationale and will proceed with the release.

The verification of the Stable Release Update for python-tornado has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package python-tornado - 4.5.3-1ubuntu0.2

---------------
python-tornado (4.5.3-1ubuntu0.2) bionic; urgency=low

  * d/p/0001-lp1903733-read-queue-of-1-message.patch (LP: #1903733)
    - fixes potential oom error with python-tornado client websocket
  * d/p/0002-lp1903733-Remove-unused-import.patch
    - code clean up

 -- Heather Lemon <email address hidden> Tue, 12 Jan 2021 20:57:40 +0000

This bug was fixed in the package python-tornado - 4.2.1-1ubuntu3.1

---------------
python-tornado (4.2.1-1ubuntu3.1) xenial; urgency=low

  * d/p/0001-lp1903733-read-queue-of-1-message.patch (LP: #1903733)
    - fixes potential oom error with python-tornado client websocket
  * d/p/0002-lp1903733-Remove-unused-import.patch
    - code clean up
  * d/control: update-maintainer

 -- Heather Lemon <email address hidden> Tue, 12 Jan 2021 21:32:45 +0000

I was asked to give this a quick look; do you have a reproducer that demonstrates the problem?

It looks to me like the bounded Queue implementation tries hard to push back against unconstrained resource growth:

    def put(
        [ ... ]
        future = Future() # type: Future[None]
        try:
            self.put_nowait(item)
        except QueueFull:
            self._putters.append((item, future))
            _set_timeout(future, timeout)
        else:
            future.set_result(None)
        return future

    def put_nowait(self, item: _T) -> None:
        [ ... ]
        self._consume_expired()
        if self._getters:
            assert self.empty(), "queue non-empty, why are getters waiting?"
            getter = self._getters.popleft()
            self.__put_internal(item)
            future_set_result_unless_cancelled(getter, self._get())
        elif self.full():
            raise QueueFull
        else:
            self.__put_internal(item)

    def full(self) -> bool:
        if self.maxsize == 0:
            return False
        else:
            return self.qsize() >= self.maxsize

Taken together, these look to me like they should raise an error if the Queue is already full, place the requestor on the _putters list, and keep going.

It's a guess, but I'd hope the size of the _putters list is constrained by per-process open file limits -- eg I'm hoping every _putter represents a socket, and those are usually constrained to 1024 per process, or 10k per process, etc.

Thanks

No, I was unable to recreate a reproducer. I attached 2 files (client.py, server.py) to demonstrate the oom error, but was unsuccessful.

| raise an error if the Queue is already full, place the requestor on the _putters list, and keep going.

Yeah I agree.

About the size of the getters and putters list. This is all I could find.
An unbounded deque,
        self._getters = collections.deque([]) # Futures.
        self._putters = collections.deque([]) # Pairs of (item, Future).

> the size of the _putters list is constrained by per-process open file limits

there is no mechanism to restrict this inherent in the Queue class, it would be up to the caller to use the class with that restriction

> every _putter represents a socket

no, their design for the Queue is for it to be used by async callers (coroutines) so there would never be more than 1 entry in the getters and/or putters.

In the specific use case modified by this patch, the WebSocketClientConnection class is updated to use the Queue class and, from its on_message method, return the future object representing the item put into the Queue. If the Queue has room for the object, its future object is the 'gen._null_future' object which is always completed (and whose result it None, but that doesn't matter). If the Queue doesn't have room, it returns a real future object that won't be completed until something calls the Queue's get() to take it off the putters deque.

The caller of WebSocketClientConnection.on_message() is thus responsible for performing the queue throttling by using the returned future object. In this case, that caller is WebSocketProtocol13._handle_message(), which returns the future provided by the call to on_message. The caller of _handle_message() is WebSocketProtocol13._on_frame_data(), that sets its local var handled_future to the future returned from the Queue. It then adds a callback to be called when the future is done, to call self._receive_frame(); in previous behavior, there was no future object returned from on_message (and thus _handle_message also returned None) so _on_frame_data() immediately called self._receive_frame(), which is what could lead to unthrottled filling of the queue.

Now that _on_frame_data() does get a future object and defers self._receive_frame() to execution only after the current message waiting in the Queue._putters deque has been removed, the Queue should never have more than a single message waiting in its putters.

I should add that while they claim their design is for async python code (e.g. coroutines) they don't actually use it from a coroutine in this older version of code, they are manually using the future's on_done_callback() instead of just await-ing the future object (as they do in newer code). However the manual addition of the callback should work just as well as a real coroutine awaiting the future object.