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

[WIP] Display descriptive error message when socket path too long #53

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Jun 10, 2022

This should fix galaxyproject/galaxy#13982

Note: stderr=subprocess.PIPE is necessary to get a non-empty return value from Popen.communicate(). I'm not sure whether that may affect anything else. A galaxy runtime error will still be logged correctly.

@jdavcs jdavcs requested review from natefoo and mvdbeek June 10, 2022 00:22
@natefoo
Copy link
Member

natefoo commented Jun 10, 2022

Thanks! Unfortunately, this causes problems with foreground mode (galaxy or galaxyctl start -f) since the process never detaches. I am not sure what the best option here is - we could check the length of the path ourselves, but that's not super portable. We could also test-create the socket in Gravity before starting supervisord.

@jdavcs
Copy link
Member Author

jdavcs commented Jun 10, 2022

Thanks! Unfortunately, this causes problems with foreground mode (galaxy or galaxyctl start -f) since the process never detaches. I am not sure what the best option here is - we could check the length of the path ourselves, but that's not super portable. We could also test-create the socket in Gravity before starting supervisord.

Yeah, that's what I was afraid of. Maybe test-create the socket first? (I thought about checking the path - but, as you said, not portable and we'd have to be conservative (103? 92 max?), so there could be many false positives)

@jdavcs
Copy link
Member Author

jdavcs commented Jun 13, 2022

@natefoo maybe something like this? (provided I fix what it's currently breaking). Or do you think we should implement what you proposed here?

Also, should we push bump to 22.09?

@jdavcs jdavcs changed the title Display descriptive error message when socket path too long [WIP] Display descriptive error message when socket path too long Jun 13, 2022
Co-authored-by: Marius van den Beek <[email protected]>
@jdavcs jdavcs force-pushed the main_socketpahtlen branch from be3b562 to 29562cb Compare June 14, 2022 20:46
sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
bind_path = self.supervisord_sock_path
try:
os.unlink(bind_path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this method is called any time the class is instantiated, so supervisord may be running and that would make this unsafe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right - this is a no-go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Cannot open an HTTP server: socket.error reported AF_UNIX path too long
4 participants