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

Classlib: Handle langPort startup error descriptively #5158

Merged
merged 1 commit into from
Oct 23, 2022

Conversation

jamshark70
Copy link
Contributor

Purpose and Motivation

Per forum thread: https://scsynth.org/t/win10-primitive-netaddr-sendmsg-failed-and-primitive-getlangport-failed/2085 -- it's possible for sclang to fail to get a UDP port at startup.

The rest of the startup sequence is omitted, which could cause any number of other problems.

Also, the error reported to the user is confusing -- a couple of users encountering this problem thought that they needed to kill servers, but it looks like it may be more relevant to kill language clients that didn't shut down.

So this PR provides more descriptive output.

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
  • This PR is ready for review

@jamshark70 jamshark70 added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library labels Sep 6, 2020
@dyfer
Copy link
Member

dyfer commented Sep 8, 2020

Thanks @jamshark70
I think this is a good improvement. I tested this and I have two comments:

  • as for wording, currently we get:
WARNING: An error occurred related to network initialization.
The error is 'ERROR: Primitive '_GetLangPort' failed.
Failed.'.
There may be an error message earlier in the sclang startup log(...)

Aside from failed.\nFailed. which seems to be an issue with the thrown error message itself, I wonder if we could either add that this is the error message, or just post the error:

WARNING: An error occurred related to network initialization.
'ERROR: Primitive '_GetLangPort' failed.
Failed.'.
There may be an error message earlier in the sclang startup log(...)
  • when I tested this first, I was not getting the message, since some of my quarks were using NetAddr.langPort, which was failing and derailing startup process. However, I tried to move super.startup after this check, and while the startup still threw an error, at least this message was displayed first. Do you think that would be too big of a change in the lang startup process? (it only moves this statement before running super.startup and interpreter.s = Server.default)
		try {
			openPorts = Set[NetAddr.langPort];
		} { |error|
			...
		};

		super.startup;
		
		// set the 's' interpreter variable to the default server.
		interpreter.s = Server.default;

EDIT:
Also, as for "there might be an error earlier"... If the error is thrown before this check, the setup is screwed anyway and this message is likely not going to be displayed. The only message displayed earlier, AFACT, is No networking: unable to bind udp socket at the very top of the post window (and easy to miss).

@mossheim mossheim removed the bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. label Oct 23, 2020
@dyfer
Copy link
Member

dyfer commented Nov 22, 2020

@jamshark70 just checking in, are you planning on following up with this PR?

@joshpar joshpar added this to the 3.13.0 milestone Feb 20, 2022
@joshpar
Copy link
Member

joshpar commented Aug 15, 2022

Hi all - we'd like to pull this in for 3.13.0 and are planning an RC for that after our meeting on Aug, 27, 2022. Anything else to add? Or can we close this?

@dyfer
Copy link
Member

dyfer commented Oct 23, 2022

Due to no further response, as well as the issue with failed\nFailed message being emitted elsewhere in the codebase, and the fact that my proposed change of order in the startup file might be too heavy for this PR, I'll pull this as is.

@dyfer dyfer merged commit 4497170 into supercollider:develop Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants