Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Fix an issue with client connection errors raised from initial request #368

Closed

Conversation

nolar
Copy link
Contributor

@nolar nolar commented May 26, 2020

What do these changes do?

Suppress all connection errors from the initial streaming request the same way as all connection errors from the continuous stream itself.

Description

An issue was caused by an aiohttp.client_exceptions.ServerDisconnectedError raised from the initial streaming request:

[2020-05-25 10:44:44,924] kopf.reactor.running [ERROR ] Root task 'watcher of pods' is failed:	
Traceback (most recent call last):	
File "/usr/local/lib/python3.7/dist-packages/kopf/reactor/running.py", line 453, in _root_task_checker	
await coro	
File "/usr/local/lib/python3.7/dist-packages/kopf/reactor/queueing.py", line 109, in watcher	
async for raw_event in stream:	
File "/usr/local/lib/python3.7/dist-packages/kopf/clients/watching.py", line 75, in infinite_watch	
async for raw_event in stream:	
File "/usr/local/lib/python3.7/dist-packages/kopf/clients/watching.py", line 112, in streaming_watch	
async for raw_event in stream:	
File "/usr/local/lib/python3.7/dist-packages/kopf/clients/watching.py", line 146, in continuous_watch	
async for raw_input in stream:	
File "/usr/local/lib/python3.7/dist-packages/kopf/clients/auth.py", line 78, in wrapper	
async for item in fn(*args, **kwargs, context=context):	
File "/usr/local/lib/python3.7/dist-packages/kopf/clients/watching.py", line 215, in watch_objs	
sock_connect=settings.watching.connect_timeout,	
File "/usr/local/lib/python3.7/dist-packages/aiohttp/client.py", line 504, in _request	
await resp.start(conn)	
File "/usr/local/lib/python3.7/dist-packages/aiohttp/client_reqrep.py", line 847, in start	
message, payload = await self._protocol.read() # type: ignore # noqa	
File "/usr/local/lib/python3.7/dist-packages/aiohttp/streams.py", line 591, in read	
await self._waiter	
aiohttp.client_exceptions.ServerDisconnectedError

The issue is not with the stream itself broken, but with the initial streaming handshake broken.

This kind of errors should be treated the same as the streaming error: i.e., by ignoring them and restarting the stream request.

Issues/PRs

Issues: fixes #369

Type of changes

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Refactoring (non-breaking change which does not alter the behaviour)
  • Mostly documentation and examples (no code changes)
  • Mostly CI/CD automation, contribution experience

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

The issue is not with the stream itself broken, but with the initial
streaming handshake broken, e.g. due to `ServerDisconnectedError`.

This kind of errors should be treated the same as the streaming error:
i.e., by ignoring them and restarting the stream request.
@nolar nolar added the bug Something isn't working label May 26, 2020
@zincr
Copy link

zincr bot commented May 26, 2020

🤖 zincr found 1 problem , 0 warnings

❌ Approvals
✅ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

@zincr
Copy link

zincr bot commented May 26, 2020

🤖 zincr found 2 problems , 0 warnings

❌ Approvals
❌ Specification
✅ Large Commits
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

Specification

All pull requests must follow certain rules for content length and form

Please ensure the follow issues are resolved:

  • ✅ Pull Request Title must be atleast 8 characters
  • ✅ Pull Request body must be atleast 8 characters
  • ❌ Pull Request body must contain issue number
     

@nolar
Copy link
Contributor Author

nolar commented Aug 20, 2020

Closed in favor of nolar/kopf#512

@nolar nolar closed this Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streaming request error caused operator crash
1 participant