-
Notifications
You must be signed in to change notification settings - Fork 204
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
StackOverflowException with attached DSN #15
Comments
And the way we got this .DSN is exactly matching #4 problem statement! |
@andrasfuchs @miho any interest to look into this bug? My PCB engineer is affected :( |
I'm already working on it ;) |
Here is my branch with the fix for this error, but it needs some testing. Could you try it? I run it with the file you provided and as you will also see in the logs, there is something strange going on with a few nets. Namely "Net-(P1-Pad36)", "Net-(P1-Pad39)", "Net-(P1-Pad40)" and "Net-(P1-Pad44)". Normally the method that caused the StackOverflowException normalizes traces, breaks them up and combines them in a way. Since I detect endless recursive loops now and I break the loop during this normalization, I'd like to ask you to double-check if the nets in question look good to you after loading your file. If they are ok, please confirm, and then we can accept this fix. If something looks fishy, I will need to come up with another solution. |
Can you please attach a .jar file here? I am just the messenger. I can forward the .jar file to my PCB engineer. and since that would not be a self-contained archive you know... that little thing. |
I think it's a not a good practice to publish executables as attachments in issues, so I would like to ask you (or your college) to download the branch I mentioned above, and follow the instructions and use Gradle to compile the code into a .jar. Let me know if you have any trouble with it, so we can find a workaround next week. |
That's fair, let me cook the jar and report the results! |
Proposed behavior abort the process, not really solves the root cause of why it fails:
|
Don't worry too much about the exception details in the log, it only indicates that the normalization process got interrupted because of the potential endless loop condition. What was the actual result from the user's perspective? Did you still get the StackOverflowException? Did you manage to open the file that you couldn't before? Were you able to do the routing on the file? Is there anything else that blocks your/your engineer's work now? If you really want to go deeper, you can do your own investigation: what are the differences between the problamatic nets I listed above and the ones that can be loaded without problems? |
@jharvey says "autorouter has worked with this newer JAR file and the original DNS file." - i.e. the fix has worked and output is still produced as expected is there a way to hide scary stacktrace from strout? it should probably be a less scary one like INFO line is this worth a unit test? |
Alright, great, thank you for the test! Sure, sorry, I forgot to change the logging level for the stdout, I thought that that exception is only visible in the more detailed log file. I'll fix that in the next release. The whole project would need a good unit testing framework, and many unit tests should be created, I'm just not sure I'm up to the task. |
from my experience it's impossible to just create all unit tests in a legacy project on the other hand creating them one by one at least every time an issues is witnessed gives a ray of hope |
Sounds good to me, feel free to create a PR with the unit test you have in mind. |
Паяльная станция.dsn and Pajalnaja_stancija.dsn is this the same file? the name is the same in Russian |
Yes, both would make a good unit test, one for the cyrillic file names, the other for the cyrillic characters inside the file. |
Let's keep this issue open until public binaries have the fix. It looks like latest 1.4.2 has the same (or different? stack over flow with the same input file. |
@andrasfuchs poke |
@rusefi I think you're right, the fix is only included in the snapshot build if I remember correctly. |
@miho Could you create a new, 1.4.3 release for us in this repo please? |
Just go to the release page and create a new one. Installers are built automatically. |
@miho @andrasfuchs do you recognize this issue by any chance? See .dsn inside attached .zip
stackOverflow.zip
The text was updated successfully, but these errors were encountered: