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

lib/osutil: Disable setting priority on Windows (fixes #4676) #4677

Closed
wants to merge 1 commit into from

Conversation

calmh
Copy link
Member

@calmh calmh commented Jan 17, 2018

Presumably fixing the crash, leaving us to improve this calmly.

Presumably fixing the crash, leaving us to improve this calmly.
@calmh
Copy link
Member Author

calmh commented Jan 17, 2018

@st-review merge

@st-review
Copy link

👌 Merged as 582539a. Thanks, @calmh!

@st-review st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 17, 2018
@st-review st-review closed this Jan 17, 2018
st-review pushed a commit that referenced this pull request Jan 17, 2018
Presumably fixing the crash, leaving us to improve this calmly.

GitHub-Pull-Request: #4677
@AudriusButkevicius
Copy link
Member

I guess this is so urgent as you want a new RC?

@calmh
Copy link
Member Author

calmh commented Jan 17, 2018

Yeah printing one currently

@calmh
Copy link
Member Author

calmh commented Jan 17, 2018

Can try to get a fixed version into an rc3, but lets not crash in the meantime

@AudriusButkevicius
Copy link
Member

I hopefully can test this, yet not until friday I suspect.

@calmh
Copy link
Member Author

calmh commented Jan 17, 2018

I can see a couple of things that might go wrong. I'm blindly assuming that kernel32.dll is available (and we do that in other places) and that SetPriorityClass is found. That may not be the case. The process handle in the crash backtrace is 0xffffffff which is suspicious (but maybe valid, I don't know, could mean "self"), yet that is one call where we do check the error. And of course I might have misunderstood something else.

@AudriusButkevicius
Copy link
Member

I suspect it crashes due to an exception/fault/whatever it's called raised during the syscall, and the exception is raised because something is not found or not valid.

@calmh calmh deleted the crash branch January 17, 2018 12:34
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jan 18, 2019
@syncthing syncthing locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion pr-merged Legacy label used in the past for pull requests merged to the main tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants