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

Close io loop before deleting attribute #27343

Merged
merged 1 commit into from Sep 24, 2015
Merged

Conversation

cachedout
Copy link
Contributor

Fixes #26889

@jfindlay jfindlay added Core relates to code central or existential to Salt Minor Change labels Sep 23, 2015
cro added a commit that referenced this pull request Sep 24, 2015
Close io loop before deleting attribute
@cro cro merged commit 331230e into saltstack:2015.8 Sep 24, 2015
skizunov added a commit to skizunov/salt that referenced this pull request Nov 5, 2015
`cls.instance_map` was growing unbounded. This was because
`io_loop.close()` was invoked before `stream.close()`. The stategy used
to fix this issue was discussed in pull request saltstack#28530 and can be
summarized as follows:

The `close()` method on the async object will be called if it exists.
This will be called before the `io_loop.close()` to give the async object
the opportunity to close its stream. The del of the async object will
continue to happen after io_loop.close(). This is to maintain the fix
from pull request saltstack#27343.

Per file change details:
salt/utils/async.py:
- Implemented the close logic described above.

salt/transport/ipc.py:
- Ensured that `close()` may be invoked more than once safely.
- In IPCServer, `self.stream` was never actually used. Removed
references to it to avoid confusion.

salt/transport/zeromq.py:
- Ensured that `close()` may be invoked more than once safely.

salt/transport/tcp.py:
- Ensured that `close()` may be invoked more than once safely.
- Changed `destroy()` methods to `close()` methods since they involved
a stream that should be closed before `io_loop.close()`.
- In SaltMessageClient, added more checks for `self._closing`. This
makes exit cleaner by not attempting to reconnect while closing.
- Removed the code associate with pull request saltstack#28113. This code is
no longer needed now that the stream is closed before the io_loop.

Signed-off-by: Sergey Kizunov <sergey.kizunov@ni.com>
rallytime pushed a commit to rallytime/salt that referenced this pull request Nov 6, 2015
`cls.instance_map` was growing unbounded. This was because
`io_loop.close()` was invoked before `stream.close()`. The stategy used
to fix this issue was discussed in pull request saltstack#28530 and can be
summarized as follows:

The `close()` method on the async object will be called if it exists.
This will be called before the `io_loop.close()` to give the async object
the opportunity to close its stream. The del of the async object will
continue to happen after io_loop.close(). This is to maintain the fix
from pull request saltstack#27343.

Per file change details:
salt/utils/async.py:
- Implemented the close logic described above.

salt/transport/ipc.py:
- Ensured that `close()` may be invoked more than once safely.
- In IPCServer, `self.stream` was never actually used. Removed
references to it to avoid confusion.

salt/transport/zeromq.py:
- Ensured that `close()` may be invoked more than once safely.

salt/transport/tcp.py:
- Ensured that `close()` may be invoked more than once safely.
- Changed `destroy()` methods to `close()` methods since they involved
a stream that should be closed before `io_loop.close()`.
- In SaltMessageClient, added more checks for `self._closing`. This
makes exit cleaner by not attempting to reconnect while closing.
- Removed the code associate with pull request saltstack#28113. This code is
no longer needed now that the stream is closed before the io_loop.

Signed-off-by: Sergey Kizunov <sergey.kizunov@ni.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core relates to code central or existential to Salt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants