-
Notifications
You must be signed in to change notification settings - Fork 469
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
Move calls to GetNewUnit into file opening routines and wrap with !$OMP critical #2538
Move calls to GetNewUnit into file opening routines and wrap with !$OMP critical #2538
Conversation
Add `!$OMP critical` around all `GetNewUnit` + `open(*)` call pairs
Also fix some hard coded file numbers
I'm not positive this is necessary without more advanced OMP directives that the `!$OMP critical`. With my luck though, some obscure compiler will be unhappy about it if I don't.
modules/nwtc-library/src/NWTC_IO.f90
Outdated
END IF | ||
|
||
!$OMP critical | ||
call GetNewUnit(Un,ErrStat,ErrMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backward compatibility, could you wrap this call to GetNewUnit (here and the other places it's now called) in an IF statement like IF (Un < 0) call GetNewUnit()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, GetNewUnit
could be changed to start looking at units at the given Un
value if it's >= 0. Otherwise, Un
is really intent(out)
only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wrap those calls in an if
statement. I think we want to maintain that backwards compatibility for now.
bccfe13
to
64316e6
Compare
64316e6
to
7c80785
Compare
7c80785
to
683ae61
Compare
After discussing this with @deslaughter, we concluded that there are to many code changes going into this PR to make sense for a 3.5.5 release. I am closing this PR and will open a new one to |
This is ready to merge
Feature or improvement description
We have had issues with unit numbers when opening files for many years when OpenMP is used. The root issue was that the
GetNewUnit
call was separate from the actual file opening. This could lead to race conditions when multiple threads hit aGetNewUnit
before the retrieved unit numbers were used to open the corresponding files.To solve this, we moved all the
GetNewUnit
calls into theOpen*File
subroutines inNWTC_IO.f90
. Inside those routines we wrap theGetNewUnit
andopen(*)
command pairs in a!$OMP critical
block. This should prevent the same unit number from being given out to two different OpenMP threads at once.We debated if this should go into 3.5.5 or 4.0.0. In the end we decided this would be better to address in 3.5.5, mostly so that I don't have to field any more questions or bug reports about it when users stay on the 3.5.x series longer.
Related issue, if one exists
#2510
There were many others over the years that we suspect may be related.
Impacted areas of the software
This change should be completely transparent to the end users, but should fix issues with conflicting unit numbers when OpenMP is used.
Additional supporting information
We originally thought about wrapping the original
GetNewUnit
calls and the followingOpen*File
calls, but decided that would complicate the code considerably. Instead we decided it was simpler to move theGetNewUnit
calls and handle the!$OMP critical
block in the library so future programmers would not need to think about it.Test results, if applicable
No test results should change.