JCluzet / 42_EXAM

A program almost identical to the 42 EXAMS for practice. (Pool EXAM & Stud EXAM)
Other
1.2k stars 89 forks source link

rank04 failing #149

Open CarloCattano opened 10 months ago

CarloCattano commented 10 months ago

I'm consistently failing microshell and Im not sure why. Could it be an issue with the tester on cluster computers or am I missing something ? Thanks for any help, I thought I was ready for the exam :-(

microshell.txt trace.txt

JCluzet commented 10 months ago

Here's what the trace shows: bin/ls: cannot access 'microshell.c': No such file or directory can you go to your rendu/ folder and run the tree command and send me the result? I think your files are not well located.

CarloCattano commented 10 months ago

pwd

/home/carlo/42/study/42_EXAM/rendu

tree


└── microshell
    └── microshell.c
CarloCattano commented 10 months ago

there was a mistake on the main, av should be av += i However it still FAILS and now it even crashed the 42-EXAM

/usr/src/debug/gcc/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.h:1329: std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::reference std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::back() [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; reference = char&]: Assertion '!empty()' failed.
.system/launch.sh: line 264: 28153 Aborted                 (core dumped) ./.system/a.out
make: *** [Makefile:14: all] Error 134
JCluzet commented 10 months ago

After a quick check, I see that there was recently a PullRequest made by @hcivici on examRank04.

@hcivici do you know if the error could be yours? Thanks :)

CarloCattano commented 10 months ago

can confirm that in commit a6233ca7278e070d533f32e156c4caa26aa61599 it passes

JCluzet commented 10 months ago

I'm going to let @hcivici fix this problem since he's the one who made the pull request, if there's no response from him I'll take care of it on my end. Thank you for reporting this 👍

for now i revert the @hcivici pull request

hcivici commented 10 months ago

Clarifications

@CarloCattano Your code fails here as seen in the trace.txt by #test1 1st and 2nd lines not being in the correct order because you are executing one command at a time. You should not use waitpid() right after every fork(). I stated some information on Fixes section for microshell.c:

By executing one command at a time: -> All the commands in the same pipeline that should have started at the same time would wait one before themself to finish executing -> Code would rely on pipe buffer capacity

You can try these commands to see it yourself:

     [shell]: sleep 1 | echo hello; sleep 1 | echo bye 
[microshell]: /bin/sleep 1 "|" /bin/echo hello ";" /bin/sleep 1 "|" /bin/echo bye

Also "cat | cat | ls should behave the normal way" -from minishell evo page. If you can make this command work as expected, you most likely won't have an fd leak. (Although you can still have duplicates of the same end of the pipe on a process which shouldn't be a problem)

Your code has other problematic behaviours such as:

I haven't touched any ls commands. As shown in the tree, microshell.c isn't in the rendu folder.

While examining the trace, I came across 65537 nulls in the file. I misread the (not very descriptive) documentation of tee command and wasn't aware that it also wrote into STDOUT along with the file(s) provided. Although it didn't (and wouldn't) cause issues (other than trace file being 64kb+1 larger :D) since a properly working rendu code would produce the same output as the internal one.

And about the crash (abort) situation, it boils down to: string, back() and Assertion '!empty()' failed. back() function returns the last character in a string. In case if you wonder what happens on an empty string, you can take a look at the implementation of back() in my machine below. image It tries to read str[-1] when string is empty After searching for back(), it turns out the only use for it is to trim trailing spaces from the line. Unrelated to ExamRank04.

I managed to reproduce the same crash with the following steps

Still not sure how you compiled the project on debug mode.

Conclusions

Solutions

CarloCattano commented 10 months ago

1- microshell.c is/was in the rendu folder 2- (You probably have re-written the whole algorithm after sending it here) Simply no , I had a mistake that I pointed out and fixed it as I said.

I understand most of what you are saying , and while yes, your pull request might be correct in all senses( apart from that crash that occurred once to me) , but frankly speaking , do you want to make the tester stricter than the exam ?

I just passed today with that solution ( not mine ) as seen here https://github.com/pasqualerossi/42-School-Exam-Rank-04/blob/main/microshell.c

should behave the normal way" -from minishell evo page. we are talking about microshell right ? I dont recall seeing such a requirement in the micro shell subject

hcivici commented 10 months ago

@CarloCattano

microshell.c is/was in the rendu folder

I mean it is not right in the rendu/ folder, it is right inside rendu/microshell/ which is the correct location. The ls test on the test.sh runs on rendu/ folder so there is no microshell.c there.

I had a mistake that I pointed out and fixed it as I said

Yes, I probably ran the old compiled version which had av += 1. Sorry about that.

do you want to make the tester stricter than the exam ? I just passed today with that solution

Not really. If Moulinette doesn't test such a thing, it becomes redundant for us to do so. Thanks for letting us know. I am going to remove the related tests from my pull request #153 (waitpid right after every fork and test_full_pipe_buffer_capacity)

I dont recall seeing such a requirement in the micro shell subject

Well, I have added fd leak tests just in case because there is a related "hint" in the subject.en.txt as follows:

Hints: Do not leak file descriptors!

Maybe this refers to leaking pipe fds in the parent process so that we would reach our max open fd limit (30) and not be able to create new pipes for the child processes.

  • Your program should be able to manage more than hundreds of "|" even if we limit the number of "open files" to less than 30.

This is already adjusted by ulimit -n 30 at the top of the test.sh Now you may ask why do I refer to minishell: If you were to run all commands in the same pipeline at the same time, cat | cat | ls would exit after you send 2 lines of input from the terminal. When ls exits, reading end of the 2nd pipe is closed, if 2nd cat tries to write() into that same pipe's write end while the read end is closed, it gets SIGPIPE signal and terminates. Same event happens for the first pipe because now 2nd cat is exited and 1st pipe's read end is closed... If you were to have an fd leak, either writing processes wouldn't get SIGPIPE or reading processes would wait for all write ends to close so that they know they can't get data from that pipe anymore when read() should have returned 0 for them. This model isn't reproduced and tested since Moulinette allows us to run 1 command at a time as you were able to pass the exam. Conclusion: FD leak only applies to parent process reaching its 30 fd limit by forgetting to close pipes 🤷 So, I am also going to remove test_fd_leak 🏳️